Skip to content

Commit

Permalink
[8.9] [Security Solution] Memoize components in create rules page (#1…
Browse files Browse the repository at this point in the history
…59635) (#160531)

# Backport

This will backport the following commits from `main` to `8.9`:
- [[Security Solution] Memoize components in create rules page
(#159635)](#159635)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Marshall
Main","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-06-26T14:34:49Z","message":"[Security
Solution] Memoize components in create rules page (#159635)\n\nAddresses
https://github.com/elastic/kibana/issues/159380\r\n\r\nChanges:\r\n-
Memoize the children of each `EuiAccordion` and the
`EuiAccordion`s\r\nthemselves on the Create rules page, as re-rendering
those accordions is\r\nexpensive\r\n- Memoize the HeaderPage component
on the create rules page\r\n- Memoize \"edit\" and \"next step\" buttons
for each step on the create\r\nrules page\r\n- Pass in specific fields
to About step instead of entire step data so\r\nit only re-renders when
relevant fields change\r\n- Move callback that sets new risk score value
when severity changes\r\nfrom `useEffect` to an `onChange` callback on
the severity field. The\r\nkey to reducing the re-rendering here is
removing the dependency on\r\n`useFormData` in the About step component,
which could have been done by\r\npassing the Severity in as a prop to
the About step component, but\r\nmoving to an onChange callback also
removes the need to pass the\r\nseverity in as a prop at all.\r\n\r\n###
Why is this so much faster?\r\nA key insight for these optimizations is
that any time the individual\r\nstep components re-render, the `Form`
component causes all of the field\r\ncomponents to re-render as well. So
the overall goal of this PR is to\r\nonly re-render the individual steps
(`StepDefineRuleComponent` and\r\n`StepAboutRuleComponent` in
particular, since they're the biggest) when\r\nnecessary. Generally,
it's necessary to re-render the form when the\r\nshape of the form
changes, i.e. we want to add or remove a field or\r\notherwise change
the way the form is presented to the user. We want to\r\n*not* re-render
the form whenever any random field in the form
changes\r\nvalue.\r\n\r\nTo accomplish this, we isolate usage of
`useFormData` to the create and\r\nedit page components and only pass in
specific necessary values from the\r\nform data to each step component.
This way the create and edit pages can\r\nre-render on every form data
change, but the steps are memoized and only\r\nre-render if an important
value changes, e.g. rule type changes and we\r\nneed to display
different fields in the define rule step.\r\n\r\nWe can still do a
better job of limiting the fields passed in to the\r\nindividual steps -
to limit the scope of this PR, I didn't aggressively\r\nrefactor the
fields that each step depends on. I only moved them to be\r\npassed in
as individual props to the steps from the page component\r\ninstead of
allowing the steps to use `useFormData`.\r\n\r\n### Future work\r\n-
Rule preview refactor and memoization\r\n- Query bar optimization\r\n-
Don't render fields that aren't visible - maybe split forms
into\r\nseparate forms for each rule type\r\n\r\n### Known issues\r\n-
Risk score track ticks overlap until the user interacts with the
risk\r\nscore
(https://github.com/elastic/eui/issues/6846)\r\n\r\n---------\r\n\r\nCo-authored-by:
Georgii Gorbachev
<[email protected]>","sha":"c64a49a1efa97a699d3706fba3fede49f638c4df","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Detections
and Resp","Team:
SecuritySolution","v8.9.0","v8.10.0"],"number":159635,"url":"https://github.com/elastic/kibana/pull/159635","mergeCommit":{"message":"[Security
Solution] Memoize components in create rules page (#159635)\n\nAddresses
https://github.com/elastic/kibana/issues/159380\r\n\r\nChanges:\r\n-
Memoize the children of each `EuiAccordion` and the
`EuiAccordion`s\r\nthemselves on the Create rules page, as re-rendering
those accordions is\r\nexpensive\r\n- Memoize the HeaderPage component
on the create rules page\r\n- Memoize \"edit\" and \"next step\" buttons
for each step on the create\r\nrules page\r\n- Pass in specific fields
to About step instead of entire step data so\r\nit only re-renders when
relevant fields change\r\n- Move callback that sets new risk score value
when severity changes\r\nfrom `useEffect` to an `onChange` callback on
the severity field. The\r\nkey to reducing the re-rendering here is
removing the dependency on\r\n`useFormData` in the About step component,
which could have been done by\r\npassing the Severity in as a prop to
the About step component, but\r\nmoving to an onChange callback also
removes the need to pass the\r\nseverity in as a prop at all.\r\n\r\n###
Why is this so much faster?\r\nA key insight for these optimizations is
that any time the individual\r\nstep components re-render, the `Form`
component causes all of the field\r\ncomponents to re-render as well. So
the overall goal of this PR is to\r\nonly re-render the individual steps
(`StepDefineRuleComponent` and\r\n`StepAboutRuleComponent` in
particular, since they're the biggest) when\r\nnecessary. Generally,
it's necessary to re-render the form when the\r\nshape of the form
changes, i.e. we want to add or remove a field or\r\notherwise change
the way the form is presented to the user. We want to\r\n*not* re-render
the form whenever any random field in the form
changes\r\nvalue.\r\n\r\nTo accomplish this, we isolate usage of
`useFormData` to the create and\r\nedit page components and only pass in
specific necessary values from the\r\nform data to each step component.
This way the create and edit pages can\r\nre-render on every form data
change, but the steps are memoized and only\r\nre-render if an important
value changes, e.g. rule type changes and we\r\nneed to display
different fields in the define rule step.\r\n\r\nWe can still do a
better job of limiting the fields passed in to the\r\nindividual steps -
to limit the scope of this PR, I didn't aggressively\r\nrefactor the
fields that each step depends on. I only moved them to be\r\npassed in
as individual props to the steps from the page component\r\ninstead of
allowing the steps to use `useFormData`.\r\n\r\n### Future work\r\n-
Rule preview refactor and memoization\r\n- Query bar optimization\r\n-
Don't render fields that aren't visible - maybe split forms
into\r\nseparate forms for each rule type\r\n\r\n### Known issues\r\n-
Risk score track ticks overlap until the user interacts with the
risk\r\nscore
(https://github.com/elastic/eui/issues/6846)\r\n\r\n---------\r\n\r\nCo-authored-by:
Georgii Gorbachev
<[email protected]>","sha":"c64a49a1efa97a699d3706fba3fede49f638c4df"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"8.9","label":"v8.9.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/159635","number":159635,"mergeCommit":{"message":"[Security
Solution] Memoize components in create rules page (#159635)\n\nAddresses
https://github.com/elastic/kibana/issues/159380\r\n\r\nChanges:\r\n-
Memoize the children of each `EuiAccordion` and the
`EuiAccordion`s\r\nthemselves on the Create rules page, as re-rendering
those accordions is\r\nexpensive\r\n- Memoize the HeaderPage component
on the create rules page\r\n- Memoize \"edit\" and \"next step\" buttons
for each step on the create\r\nrules page\r\n- Pass in specific fields
to About step instead of entire step data so\r\nit only re-renders when
relevant fields change\r\n- Move callback that sets new risk score value
when severity changes\r\nfrom `useEffect` to an `onChange` callback on
the severity field. The\r\nkey to reducing the re-rendering here is
removing the dependency on\r\n`useFormData` in the About step component,
which could have been done by\r\npassing the Severity in as a prop to
the About step component, but\r\nmoving to an onChange callback also
removes the need to pass the\r\nseverity in as a prop at all.\r\n\r\n###
Why is this so much faster?\r\nA key insight for these optimizations is
that any time the individual\r\nstep components re-render, the `Form`
component causes all of the field\r\ncomponents to re-render as well. So
the overall goal of this PR is to\r\nonly re-render the individual steps
(`StepDefineRuleComponent` and\r\n`StepAboutRuleComponent` in
particular, since they're the biggest) when\r\nnecessary. Generally,
it's necessary to re-render the form when the\r\nshape of the form
changes, i.e. we want to add or remove a field or\r\notherwise change
the way the form is presented to the user. We want to\r\n*not* re-render
the form whenever any random field in the form
changes\r\nvalue.\r\n\r\nTo accomplish this, we isolate usage of
`useFormData` to the create and\r\nedit page components and only pass in
specific necessary values from the\r\nform data to each step component.
This way the create and edit pages can\r\nre-render on every form data
change, but the steps are memoized and only\r\nre-render if an important
value changes, e.g. rule type changes and we\r\nneed to display
different fields in the define rule step.\r\n\r\nWe can still do a
better job of limiting the fields passed in to the\r\nindividual steps -
to limit the scope of this PR, I didn't aggressively\r\nrefactor the
fields that each step depends on. I only moved them to be\r\npassed in
as individual props to the steps from the page component\r\ninstead of
allowing the steps to use `useFormData`.\r\n\r\n### Future work\r\n-
Rule preview refactor and memoization\r\n- Query bar optimization\r\n-
Don't render fields that aren't visible - maybe split forms
into\r\nseparate forms for each rule type\r\n\r\n### Known issues\r\n-
Risk score track ticks overlap until the user interacts with the
risk\r\nscore
(https://github.com/elastic/eui/issues/6846)\r\n\r\n---------\r\n\r\nCo-authored-by:
Georgii Gorbachev
<[email protected]>","sha":"c64a49a1efa97a699d3706fba3fede49f638c4df"}}]}]
BACKPORT-->

Co-authored-by: Marshall Main <[email protected]>
  • Loading branch information
kibanamachine and marshallmain authored Jun 26, 2023
1 parent b011f2a commit 25fa4fd
Show file tree
Hide file tree
Showing 12 changed files with 644 additions and 433 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,13 @@ export const useRuleForms = ({
});
// FormData doesn't populate on the first render, so we use the defaultValue if the formData
// doesn't have what we wanted
const defineStepData =
'index' in defineStepFormData
? { ...defineStepFormData, eqlOptions: eqlOptionsSelected }
: defineStepDefault;
const defineStepData = useMemo(
() =>
'index' in defineStepFormData
? { ...defineStepFormData, eqlOptions: eqlOptionsSelected }
: defineStepDefault,
[defineStepDefault, defineStepFormData, eqlOptionsSelected]
);

// ABOUT STEP FORM
const typeDependentAboutRuleSchema = useMemo(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { EuiButton } from '@elastic/eui';
import type { EuiResizableContainerActions } from '@elastic/eui/src/components/resizable_container/types';
import React, { memo } from 'react';
import * as i18n from './translations';
import { HeaderPage } from '../../../common/components/header_page';
import type { HeaderPageProps } from '../../../common/components/header_page';

const CustomHeaderPage: React.FC<
HeaderPageProps & {
togglePanel: EuiResizableContainerActions['togglePanel'] | undefined;
isRulePreviewVisible: boolean;
setIsRulePreviewVisible: (value: React.SetStateAction<boolean>) => void;
}
> = ({
backOptions,
isLoading,
title,
togglePanel,
isRulePreviewVisible,
setIsRulePreviewVisible,
}) => (
<HeaderPage backOptions={backOptions} isLoading={isLoading} title={title}>
<EuiButton
data-test-subj="preview-container"
isSelected={isRulePreviewVisible}
fill={isRulePreviewVisible}
iconType="visBarVerticalStacked"
onClick={() => {
togglePanel?.('preview', { direction: 'left' });
setIsRulePreviewVisible((isVisible) => !isVisible);
}}
>
{i18n.RULE_PREVIEW_TITLE}
</EuiButton>
</HeaderPage>
);

export const CustomHeaderPageMemo = memo(CustomHeaderPage);
Loading

0 comments on commit 25fa4fd

Please sign in to comment.