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.

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

[Security Solution] Rule details page is not responsive and leads to page crash for rule in non-default spaces #177734

Closed
vitaliidm opened this issue Feb 23, 2024 · 19 comments
Assignees
Labels
8.15 candidate bug Fixes for quality problems that affect the customer experience Feature:Rule Details Security Solution Detection Rule Details page fixed impact:critical This issue should be addressed immediately due to a critical level of impact on the product. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.13.0 v8.14.0 v8.15.0

Comments

@vitaliidm
Copy link
Contributor

Describe the bug:

Rule details page is not responsive and leads to page crash for rule in non-default spaces

Kibana/Elasticsearch Stack version:
8.13

Server OS version:

Browser and Browser OS versions:

Elastic Endpoint version:

Original install method (e.g. download page, yum, from source, etc.):

Functional Area (e.g. Endpoint management, timelines, resolver, etc.):

Detection rules details page
Steps to reproduce:

  1. Create detection rule in space that is not default
  2. Open its rule details
  3. Observe indefinite page loading, page is not responsive as well. Eventually crashes

Rule details page works as expected in default space

Any additional context (logs, chat logs, magical formulas, etc.):

Screen.Recording.2024-02-23.at.15.40.11.mov
@vitaliidm vitaliidm added bug Fixes for quality problems that affect the customer experience triage_needed Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Feb 23, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@vitaliidm vitaliidm added Team:Detections and Resp Security Detection Response Team Team:Detection Rule Management Security Detection Rule Management Team labels Feb 23, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@maximpn maximpn self-assigned this Feb 23, 2024
@banderror banderror added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Feature:Rule Details Security Solution Detection Rule Details page 8.13 candidate and removed triage_needed labels Feb 23, 2024
@shayfeld
Copy link

shayfeld commented Feb 24, 2024

Hi @vitaliidm @banderror @maximpn ,
Is it only happening with rules that have a custom query "*"?

@maximpn
Copy link
Contributor

maximpn commented Feb 25, 2024

@shayfeld The problem happens for any kind of queries. Investigation has shown it's not related to rules but rather to the alerts chart. I've prepared a fix PR #177770.

@mbudge
Copy link

mbudge commented Feb 25, 2024

Does this affect 8.12.0 to 8.12.2?

it looks like it would prevent us building new rules in the non-default space.

just want to check so we don’t upgrade from 8.11.4

maximpn added a commit that referenced this issue Feb 26, 2024
… rerendering (#177770)

**Fixes:** #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).
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Feb 26, 2024
… 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)
kibanamachine referenced this issue Feb 27, 2024
…nfinite rerendering (#177770) (#177805)

# Backport

This will backport the following commits from `main` to `8.13`:
- [[Security Solution] Fix VisualizationActions React component infinite
rerendering (#177770)](#177770)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-02-26T23:06:25Z","message":"[Security
Solution] Fix VisualizationActions React component infinite rerendering
(#177770)\n\n**Fixes:**
https://github.com/elastic/kibana/issues/177734\r\n\r\n##
Summary\r\n\r\nThis PR fixes `VisualizationActions` React component
infinite rerendering caused by unstable hook dependencies and async
state update via `useAsync`.\r\n\r\n## Details\r\n\r\nInitial 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).\r\n\r\nBesides Rule
details page the problem occurs on alerts page as well.\r\n\r\n###
Promise + unstable dependencies = infinite re-render\r\n\r\nReact
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.\r\n\r\nHow does it
work\r\n\r\n- a hook or some function returns a promise\r\n- promise
result is awaited in `useEffect` + `.then()` or by using `useAsync`\r\n-
after a promise resolves a new state is set (state is needed to rendered
the promise result)\r\n- state update leads to a new render where the
cycle repeats\r\n\r\n### What are the typical mistakes with
dependencies\r\n\r\n- Accepting an optional parameter and this parameter
has a default value as an empty object or array\r\n\r\n```ts\r\nfunction
myHook(param = {}) {\r\n ...\r\n\r\n return useMemo(...,
[param]);\r\n}\r\n```\r\n\r\n- Accepting a configuration object which is
used as dependency\r\n\r\n```ts\r\nfunction myHook(params) {\r\n
...\r\n\r\n return useMemo(..., [params]);\r\n}\r\n\r\nfunction
MyComponent() {\r\n const result = myHook({\r\n someOption: 'foo',\r\n
});\r\n}\r\n```\r\n\r\n- Returning an object without memoization (while
field values could be memoized)\r\n\r\n```ts\r\nfunction myHook(params)
{\r\n const cb = useCallback(() => {...}, []);\r\n\r\n return {\r\n foo:
cb,\r\n };\r\n}\r\n\r\nfunction MyComponent() {\r\n const result =
myHook();\r\n\r\n const memoizedResult = useMemo(() => result,
[result]);\r\n}\r\n```\r\n\r\n- Use the whole input properties object as
a dependency\r\n\r\nSpreading 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).\r\n\r\n```ts\r\nfunction
MyComponent(props) {\r\n const myCallback = useCallback(() => {\r\n
invokeExternalFunction({\r\n ...props,\r\n foo: 'bar',\r\n });\r\n },
[props]);\r\n\r\n ...\r\n}\r\n```\r\n\r\n### Fixes in this
PR\r\n\r\nThis PR updates involved hooks to make them producing stable
results (for example object or array isn't regenerated every
render).","sha":"d45b8d3ab3cc8c82f18526678d9a66d22fb9de38","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Rule
Details","v8.13.0","v8.14.0"],"title":"[Security Solution] Fix
VisualizationActions React component infinite
rerendering","number":177770,"url":"https://github.com/elastic/kibana/pull/177770","mergeCommit":{"message":"[Security
Solution] Fix VisualizationActions React component infinite rerendering
(#177770)\n\n**Fixes:**
https://github.com/elastic/kibana/issues/177734\r\n\r\n##
Summary\r\n\r\nThis PR fixes `VisualizationActions` React component
infinite rerendering caused by unstable hook dependencies and async
state update via `useAsync`.\r\n\r\n## Details\r\n\r\nInitial 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).\r\n\r\nBesides Rule
details page the problem occurs on alerts page as well.\r\n\r\n###
Promise + unstable dependencies = infinite re-render\r\n\r\nReact
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.\r\n\r\nHow does it
work\r\n\r\n- a hook or some function returns a promise\r\n- promise
result is awaited in `useEffect` + `.then()` or by using `useAsync`\r\n-
after a promise resolves a new state is set (state is needed to rendered
the promise result)\r\n- state update leads to a new render where the
cycle repeats\r\n\r\n### What are the typical mistakes with
dependencies\r\n\r\n- Accepting an optional parameter and this parameter
has a default value as an empty object or array\r\n\r\n```ts\r\nfunction
myHook(param = {}) {\r\n ...\r\n\r\n return useMemo(...,
[param]);\r\n}\r\n```\r\n\r\n- Accepting a configuration object which is
used as dependency\r\n\r\n```ts\r\nfunction myHook(params) {\r\n
...\r\n\r\n return useMemo(..., [params]);\r\n}\r\n\r\nfunction
MyComponent() {\r\n const result = myHook({\r\n someOption: 'foo',\r\n
});\r\n}\r\n```\r\n\r\n- Returning an object without memoization (while
field values could be memoized)\r\n\r\n```ts\r\nfunction myHook(params)
{\r\n const cb = useCallback(() => {...}, []);\r\n\r\n return {\r\n foo:
cb,\r\n };\r\n}\r\n\r\nfunction MyComponent() {\r\n const result =
myHook();\r\n\r\n const memoizedResult = useMemo(() => result,
[result]);\r\n}\r\n```\r\n\r\n- Use the whole input properties object as
a dependency\r\n\r\nSpreading 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).\r\n\r\n```ts\r\nfunction
MyComponent(props) {\r\n const myCallback = useCallback(() => {\r\n
invokeExternalFunction({\r\n ...props,\r\n foo: 'bar',\r\n });\r\n },
[props]);\r\n\r\n ...\r\n}\r\n```\r\n\r\n### Fixes in this
PR\r\n\r\nThis PR updates involved hooks to make them producing stable
results (for example object or array isn't regenerated every
render).","sha":"d45b8d3ab3cc8c82f18526678d9a66d22fb9de38"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","branchLabelMappingKey":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/177770","number":177770,"mergeCommit":{"message":"[Security
Solution] Fix VisualizationActions React component infinite rerendering
(#177770)\n\n**Fixes:**
https://github.com/elastic/kibana/issues/177734\r\n\r\n##
Summary\r\n\r\nThis PR fixes `VisualizationActions` React component
infinite rerendering caused by unstable hook dependencies and async
state update via `useAsync`.\r\n\r\n## Details\r\n\r\nInitial 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).\r\n\r\nBesides Rule
details page the problem occurs on alerts page as well.\r\n\r\n###
Promise + unstable dependencies = infinite re-render\r\n\r\nReact
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.\r\n\r\nHow does it
work\r\n\r\n- a hook or some function returns a promise\r\n- promise
result is awaited in `useEffect` + `.then()` or by using `useAsync`\r\n-
after a promise resolves a new state is set (state is needed to rendered
the promise result)\r\n- state update leads to a new render where the
cycle repeats\r\n\r\n### What are the typical mistakes with
dependencies\r\n\r\n- Accepting an optional parameter and this parameter
has a default value as an empty object or array\r\n\r\n```ts\r\nfunction
myHook(param = {}) {\r\n ...\r\n\r\n return useMemo(...,
[param]);\r\n}\r\n```\r\n\r\n- Accepting a configuration object which is
used as dependency\r\n\r\n```ts\r\nfunction myHook(params) {\r\n
...\r\n\r\n return useMemo(..., [params]);\r\n}\r\n\r\nfunction
MyComponent() {\r\n const result = myHook({\r\n someOption: 'foo',\r\n
});\r\n}\r\n```\r\n\r\n- Returning an object without memoization (while
field values could be memoized)\r\n\r\n```ts\r\nfunction myHook(params)
{\r\n const cb = useCallback(() => {...}, []);\r\n\r\n return {\r\n foo:
cb,\r\n };\r\n}\r\n\r\nfunction MyComponent() {\r\n const result =
myHook();\r\n\r\n const memoizedResult = useMemo(() => result,
[result]);\r\n}\r\n```\r\n\r\n- Use the whole input properties object as
a dependency\r\n\r\nSpreading 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).\r\n\r\n```ts\r\nfunction
MyComponent(props) {\r\n const myCallback = useCallback(() => {\r\n
invokeExternalFunction({\r\n ...props,\r\n foo: 'bar',\r\n });\r\n },
[props]);\r\n\r\n ...\r\n}\r\n```\r\n\r\n### Fixes in this
PR\r\n\r\nThis PR updates involved hooks to make them producing stable
results (for example object or array isn't regenerated every
render).","sha":"d45b8d3ab3cc8c82f18526678d9a66d22fb9de38"}}]}]
BACKPORT-->

Co-authored-by: Maxim Palenov <[email protected]>
@banderror
Copy link
Contributor

@mbudge This shouldn't be broken in 8.12.x because it looks like the bug was introduced by #173884 which went to 8.13.0 only.

@banderror banderror added impact:critical This issue should be addressed immediately due to a critical level of impact on the product. fixed v8.13.0 v8.14.0 and removed impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Feb 27, 2024
@maximpn
Copy link
Contributor

maximpn commented Feb 27, 2024

@mbudge as @banderror mentioned it's safe to upgrade to 8.12.x. I double checked on 8.12.2 just in case and everything is fine.

@banderror
Copy link
Contributor

@vgomez-el Could you please validate the fix (#177770) in the next 8.13.0 BC (BC3 Feb 29)? Thank you!

@leandrojmp
Copy link

Hello,

Could this issue #177838 I opened be related to this one? We are on 8.12.1.

I'm using the default space, but cannot edit, add or remove any filter on my custom rules. Also, when opening a rule to edit the filters take a couple of seconds to load.

This also happens while creating new rules.

@maximpn
Copy link
Contributor

maximpn commented Mar 1, 2024

Hi @leandrojmp,

I can't say the current issues is related to yours #177838. The problem here is the page is hung and non responsible at al. In your case it's possible to interact with UI and the problem in something else like a bug or a performance issue.

@vgomez-el vgomez-el added the QA:Validated Issue has been validated by QA label Mar 1, 2024
@vgomez-el
Copy link

Bug is fixed and validated in 8.13 BC3. thank you @maximpn and @banderror for the fix!

REC-20240301135414.mp4

semd pushed a commit to semd/kibana that referenced this issue Mar 4, 2024
… 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).
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
… 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).
@vitaliidm
Copy link
Contributor Author

Looks like this issue resurfaced again in 8.15

On latest main and latest 8.15 snapshot in cloud I see the following behaviour

Screen.Recording.2024-07-04.at.12.17.45.mov

Works as expected in 8.14.2

@vitaliidm vitaliidm reopened this Jul 4, 2024
@MadameSheema MadameSheema removed the fixed label Jul 8, 2024
@banderror banderror added 8.15 candidate v8.15.0 and removed QA:Validated Issue has been validated by QA 8.13 candidate labels Jul 8, 2024
maximpn added a commit that referenced this issue Jul 15, 2024
**Resolves:** #177734

## Summary

This PR fixes a non-responsive rule details page under non default space.

## Details

**[Security Solution] Rule details page is not responsive and leads to page crash for rule in non-default spaces [#177734](#177734 resurfaced back. Investigation has show that **[Security Solution] Remove usage of deprecated React rendering utilities [#181099](#181099 is the cause.

The problem is quite subtle to comprehend it just by looking at the code. In fact it boils down to an unstable `useAsync()` hook dependency. Every re-render `useAsync()` resolves a promise causing an additional re-render to show updated results and the cycle repeats. Such hook is used in `x-pack/plugins/security_solution/public/common/components/visualization_actions/actions.tsx`

```ts
  const panels = useAsync(
    () =>
      buildContextMenuForActions({
        actions: contextMenuActions.map((action) => ({
          action,
          context: {},
          trigger: VISUALIZATION_CONTEXT_MENU_TRIGGER,
        })),
      }),
    [contextMenuActions]
  );
```

where `contextMenuActions` is an unstable dependency. This is the case due to refactoring to `useSaveToLibrary()` hook by **[Security Solution] Remove usage of deprecated React rendering utilities [#181099](#181099 which started retuning a new object every render. The dependency chain is  `contextMenuActions` -> `useActions()` -> `useSaveToLibrary()`.

The actual fix is to replace

```ts
const { lens, ...startServices } = useKibana().services;
```

with

```ts
const startServices = useKibana().services;
```

Since `startServices` is used as a hook dependency it must be stable. A rest property in object destruction expression is always a new object and can't be used as a dependency as is. Using stable `useKibana().services` fixes the problem.
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Jul 15, 2024
)

**Resolves:** elastic#177734

## Summary

This PR fixes a non-responsive rule details page under non default space.

## Details

**[Security Solution] Rule details page is not responsive and leads to page crash for rule in non-default spaces [elastic#177734](elastic#177734 resurfaced back. Investigation has show that **[Security Solution] Remove usage of deprecated React rendering utilities [elastic#181099](elastic#181099 is the cause.

The problem is quite subtle to comprehend it just by looking at the code. In fact it boils down to an unstable `useAsync()` hook dependency. Every re-render `useAsync()` resolves a promise causing an additional re-render to show updated results and the cycle repeats. Such hook is used in `x-pack/plugins/security_solution/public/common/components/visualization_actions/actions.tsx`

```ts
  const panels = useAsync(
    () =>
      buildContextMenuForActions({
        actions: contextMenuActions.map((action) => ({
          action,
          context: {},
          trigger: VISUALIZATION_CONTEXT_MENU_TRIGGER,
        })),
      }),
    [contextMenuActions]
  );
```

where `contextMenuActions` is an unstable dependency. This is the case due to refactoring to `useSaveToLibrary()` hook by **[Security Solution] Remove usage of deprecated React rendering utilities [elastic#181099](elastic#181099 which started retuning a new object every render. The dependency chain is  `contextMenuActions` -> `useActions()` -> `useSaveToLibrary()`.

The actual fix is to replace

```ts
const { lens, ...startServices } = useKibana().services;
```

with

```ts
const startServices = useKibana().services;
```

Since `startServices` is used as a hook dependency it must be stable. A rest property in object destruction expression is always a new object and can't be used as a dependency as is. Using stable `useKibana().services` fixes the problem.

(cherry picked from commit 8a539a8)
kibanamachine added a commit that referenced this issue Jul 15, 2024
) (#188305)

# Backport

This will backport the following commits from `main` to `8.15`:
- [[Security Solution] Fix non-responsive rule details page
(#187953)](#187953)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-07-15T11:37:26Z","message":"[Security
Solution] Fix non-responsive rule details page
(#187953)\n\n**Resolves:**
https://github.com/elastic/kibana/issues/177734\r\n\r\n##
Summary\r\n\r\nThis PR fixes a non-responsive rule details page under
non default space.\r\n\r\n## Details\r\n\r\n**[Security Solution] Rule
details page is not responsive and leads to page crash for rule in
non-default spaces
[#177734](#177734 resurfaced
back. Investigation has show that **[Security Solution] Remove usage of
deprecated React rendering utilities
[#181099](#181099 is the
cause.\r\n\r\nThe problem is quite subtle to comprehend it just by
looking at the code. In fact it boils down to an unstable `useAsync()`
hook dependency. Every re-render `useAsync()` resolves a promise causing
an additional re-render to show updated results and the cycle repeats.
Such hook is used in
`x-pack/plugins/security_solution/public/common/components/visualization_actions/actions.tsx`\r\n\r\n```ts\r\n
const panels = useAsync(\r\n () =>\r\n buildContextMenuForActions({\r\n
actions: contextMenuActions.map((action) => ({\r\n action,\r\n context:
{},\r\n trigger: VISUALIZATION_CONTEXT_MENU_TRIGGER,\r\n })),\r\n
}),\r\n [contextMenuActions]\r\n );\r\n```\r\n\r\nwhere
`contextMenuActions` is an unstable dependency. This is the case due to
refactoring to `useSaveToLibrary()` hook by **[Security Solution] Remove
usage of deprecated React rendering utilities
[#181099](#181099 which started
retuning a new object every render. The dependency chain is
`contextMenuActions` -> `useActions()` ->
`useSaveToLibrary()`.\r\n\r\nThe actual fix is to
replace\r\n\r\n```ts\r\nconst { lens, ...startServices } =
useKibana().services;\r\n```\r\n\r\nwith\r\n\r\n```ts\r\nconst
startServices = useKibana().services;\r\n```\r\n\r\nSince
`startServices` is used as a hook dependency it must be stable. A rest
property in object destruction expression is always a new object and
can't be used as a dependency as is. Using stable `useKibana().services`
fixes the
problem.","sha":"8a539a8a4ce483d2e5d3aa484d8ec78887f338b8","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","impact:critical","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Rule
Details","v8.15.0","v8.16.0"],"title":"[Security Solution] Fix
non-responsive rule details
page","number":187953,"url":"https://github.com/elastic/kibana/pull/187953","mergeCommit":{"message":"[Security
Solution] Fix non-responsive rule details page
(#187953)\n\n**Resolves:**
https://github.com/elastic/kibana/issues/177734\r\n\r\n##
Summary\r\n\r\nThis PR fixes a non-responsive rule details page under
non default space.\r\n\r\n## Details\r\n\r\n**[Security Solution] Rule
details page is not responsive and leads to page crash for rule in
non-default spaces
[#177734](#177734 resurfaced
back. Investigation has show that **[Security Solution] Remove usage of
deprecated React rendering utilities
[#181099](#181099 is the
cause.\r\n\r\nThe problem is quite subtle to comprehend it just by
looking at the code. In fact it boils down to an unstable `useAsync()`
hook dependency. Every re-render `useAsync()` resolves a promise causing
an additional re-render to show updated results and the cycle repeats.
Such hook is used in
`x-pack/plugins/security_solution/public/common/components/visualization_actions/actions.tsx`\r\n\r\n```ts\r\n
const panels = useAsync(\r\n () =>\r\n buildContextMenuForActions({\r\n
actions: contextMenuActions.map((action) => ({\r\n action,\r\n context:
{},\r\n trigger: VISUALIZATION_CONTEXT_MENU_TRIGGER,\r\n })),\r\n
}),\r\n [contextMenuActions]\r\n );\r\n```\r\n\r\nwhere
`contextMenuActions` is an unstable dependency. This is the case due to
refactoring to `useSaveToLibrary()` hook by **[Security Solution] Remove
usage of deprecated React rendering utilities
[#181099](#181099 which started
retuning a new object every render. The dependency chain is
`contextMenuActions` -> `useActions()` ->
`useSaveToLibrary()`.\r\n\r\nThe actual fix is to
replace\r\n\r\n```ts\r\nconst { lens, ...startServices } =
useKibana().services;\r\n```\r\n\r\nwith\r\n\r\n```ts\r\nconst
startServices = useKibana().services;\r\n```\r\n\r\nSince
`startServices` is used as a hook dependency it must be stable. A rest
property in object destruction expression is always a new object and
can't be used as a dependency as is. Using stable `useKibana().services`
fixes the
problem.","sha":"8a539a8a4ce483d2e5d3aa484d8ec78887f338b8"}},"sourceBranch":"main","suggestedTargetBranches":["8.15"],"targetPullRequestStates":[{"branch":"8.15","label":"v8.15.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/187953","number":187953,"mergeCommit":{"message":"[Security
Solution] Fix non-responsive rule details page
(#187953)\n\n**Resolves:**
https://github.com/elastic/kibana/issues/177734\r\n\r\n##
Summary\r\n\r\nThis PR fixes a non-responsive rule details page under
non default space.\r\n\r\n## Details\r\n\r\n**[Security Solution] Rule
details page is not responsive and leads to page crash for rule in
non-default spaces
[#177734](#177734 resurfaced
back. Investigation has show that **[Security Solution] Remove usage of
deprecated React rendering utilities
[#181099](#181099 is the
cause.\r\n\r\nThe problem is quite subtle to comprehend it just by
looking at the code. In fact it boils down to an unstable `useAsync()`
hook dependency. Every re-render `useAsync()` resolves a promise causing
an additional re-render to show updated results and the cycle repeats.
Such hook is used in
`x-pack/plugins/security_solution/public/common/components/visualization_actions/actions.tsx`\r\n\r\n```ts\r\n
const panels = useAsync(\r\n () =>\r\n buildContextMenuForActions({\r\n
actions: contextMenuActions.map((action) => ({\r\n action,\r\n context:
{},\r\n trigger: VISUALIZATION_CONTEXT_MENU_TRIGGER,\r\n })),\r\n
}),\r\n [contextMenuActions]\r\n );\r\n```\r\n\r\nwhere
`contextMenuActions` is an unstable dependency. This is the case due to
refactoring to `useSaveToLibrary()` hook by **[Security Solution] Remove
usage of deprecated React rendering utilities
[#181099](#181099 which started
retuning a new object every render. The dependency chain is
`contextMenuActions` -> `useActions()` ->
`useSaveToLibrary()`.\r\n\r\nThe actual fix is to
replace\r\n\r\n```ts\r\nconst { lens, ...startServices } =
useKibana().services;\r\n```\r\n\r\nwith\r\n\r\n```ts\r\nconst
startServices = useKibana().services;\r\n```\r\n\r\nSince
`startServices` is used as a hook dependency it must be stable. A rest
property in object destruction expression is always a new object and
can't be used as a dependency as is. Using stable `useKibana().services`
fixes the
problem.","sha":"8a539a8a4ce483d2e5d3aa484d8ec78887f338b8"}}]}]
BACKPORT-->

Co-authored-by: Maxim Palenov <[email protected]>
@banderror
Copy link
Contributor

@pborgonovi The bug was fixed by @maximpn in #187953 and backported to 8.15 in #188305. Could you please see if you can help us validate this bugfix? Feel free to close this ticket at will.

@pborgonovi
Copy link
Contributor

pborgonovi commented Jul 15, 2024

Hello @maximpn @banderror I tested the fix with latest 8.15 BC and below is what I got:

  • First attempt creating a new rule in a custom space: the page is not responsive and crashes after some seconds:
Bug-Creating.new.rule.mov
  • I reproduced the exact same steps some time after: error doesn't occur:
Bug.-.Create.new.rule.2.OK.mov
  • Created new rules on custom space in different ways (e.g: editing index patterns, not editing...): everything worked fine.
Bug-Create.new.rule.remove.index.pattern.mov
Bug.-.OK.2.mov
Bug.-.OK.2.mov

I could isolate the problem and came to a final conclusion:
Issue is happening when creating a rule on a new custom space when space name hasn't been previously used. See evidences below:

Bug-reproduced.1.mov
Bug.-.reproduced.2.mov

@maximpn maximpn added ci:cloud-deploy Create or update a Cloud deployment ci:project-deploy-security Create a Security Serverless Project and removed ci:cloud-deploy Create or update a Cloud deployment ci:project-deploy-security Create a Security Serverless Project labels Jul 16, 2024
@maximpn
Copy link
Contributor

maximpn commented Jul 16, 2024

@pborgonovi thank you for testing and spotting a problem. I'll have a look and adjust the added Cypress test to have it covered.

@maximpn
Copy link
Contributor

maximpn commented Jul 16, 2024

@pborgonovi

Ok, as I suspected my fix didn’t make it to the latest 8.15 snapshot. It was run ~2 hours earlier than the backport got to 8.15 branch. It makes sense to redeploy 8.15 snapshot later today to perform testing.

Btw 8.16 snahpshot publishing pipeline is broken since last Wednesday which means changes won’t get to snapshots.

The safest option is to deploy via PR labels since it guarantees to deploy specific code.

@pborgonovi
Copy link
Contributor

@maximpn @banderror

I've retested the fix and couldn't get any issues. Below are the evidences of new rules created on custom spaces with names not previously used:

Fix backported to 8.15:

Bug-Fix_8.15.mov

8.16:

Bug-Fix_8.16.mov

I'm closing this as resolved.

@pborgonovi
Copy link
Contributor

Issue is resolved as per comment above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.15 candidate bug Fixes for quality problems that affect the customer experience Feature:Rule Details Security Solution Detection Rule Details page fixed impact:critical This issue should be addressed immediately due to a critical level of impact on the product. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.13.0 v8.14.0 v8.15.0
Projects
None yet
Development

No branches or pull requests

10 participants