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

Jump to bottom

[Security Solution] Exceptions flyout refreshes when it regains focus, causing configured fields to get cleared (#166550) #172666

Merged
merged 5 commits into from
Dec 12, 2023
Merged

[Security Solution] Exceptions flyout refreshes when it regains focus, causing configured fields to get cleared (#166550) #172666

merged 5 commits into from
Dec 12, 2023

Conversation

e40pud
Copy link
Contributor

@e40pud e40pud commented Dec 6, 2023 •
edited by kibanamachine
Loading

Summary

Addresses #166550

These changes fix the issue where user can experience data loss while adding rule exceptions.

The main issue is that we keep conditions state inside the child component ExceptionsConditions which is rendered conditionally based on isLoading flag. This flag changes when useQuery data gets stale and refetching is triggered. In this case we would remove ExceptionsConditions component while loading data and re-create it after. Since conditions state stored inside ExceptionsConditions we will lose it.

This is a quick fix to make sure our users are not frustrated. The state refactoring will come separately in the next release when we are going to address the main issue https://github.com/elastic/security-team/issues/8197

To reproduce:

  1. Open "add rule exception" flyout
  2. Add exception conditions
  3. Wait for 5 minutes (to avoid waiting this long you can use this approach)
  4. Remove focus from the page (by switching to another app or navigating to a different tab in a browser)
  5. Focus on the page again

When you are back to the page all exception conditions should still be there.

@e40pud e40pud added release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Engine Security Solution Detection Engine Area labels Dec 6, 2023
@e40pud e40pud self-assigned this Dec 6, 2023
@e40pud e40pud requested a review from a team as a code owner December 6, 2023 11:15
@e40pud e40pud requested a review from vitaliidm December 6, 2023 11:15
@elasticmachine
Copy link
Contributor

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

@e40pud e40pud requested a review from yctercero December 6, 2023 11:15
vitaliidm
vitaliidm approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@vitaliidm vitaliidm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for fixing this.

As this is intended as a hot fix and further refactoring ticket is created I think it's fine, but wanted just to highlight, with the fix both loader and form is visible at the same time.

Screen.Recording.2023-12-07.at.11.35.21.mov

We might want to consider, to wrap form in into a component and use css visibility: hidden property on it, when isLoading is true. This way, form controllers won't be re-rendered and won't lose their state, but UI will be much more friendlier.
What do you think, @e40pud ?

...y_solution/public/detection_engine/rule_exceptions/components/add_exception_flyout/index.tsx Outdated Show resolved Hide resolved
@e40pud
Copy link
Contributor Author

e40pud commented Dec 7, 2023 •
edited
Loading

Thanks for fixing this.

As this is intended as a hot fix and further refactoring ticket is created I think it's fine, but wanted just to highlight, with the fix both loader and form is visible at the same time.

Screen.Recording.2023-12-07.at.11.35.21.mov

We might want to consider, to wrap form in into a component and use css visible: hidden property on it, when isLoading is true. This way, form controllers won't be re-rendered and won't lose their state, but UI will be much more friendlier. What do you think, @e40pud ?

Yeah, we discussed this with @yctercero and decided to make a hot fix only. We did not want to focus on making UI changes to improve the experience, and rather do a proper fix in the next release. We have exactly the same behaviour in EditExceptionFlyout and it is there already for a while, so that issue with both loading and actual content being present at the same time is already in the code.

@e40pud e40pud added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Dec 7, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Detection Engine - Security Solution Cypress Tests #6 / Alert user assignment - ESS & Serverless Basic rendering alert with many assignees (collapsed into badge) in alerts table alert with many assignees (collapsed into badge) in alerts table

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 13.1MB 13.1MB -9.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @e40pud

@e40pud e40pud merged commit a0631cf into elastic:main Dec 12, 2023
38 checks passed
@kibanamachine kibanamachine mentioned this pull request Dec 12, 2023
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 12, 2023
@e40pud
[Security Solution] Exceptions flyout refreshes when it regains focus…
f4a63f2 
…, causing configured fields to get cleared (elastic#166550) (elastic#172666)

## Summary

Addresses elastic#166550

These changes fix the issue where user can experience data loss while
adding rule exceptions.

The main issue is that we keep conditions state inside the child
component `ExceptionsConditions` which is rendered conditionally based
on `isLoading` flag. This flag changes when `useQuery` data gets stale
and refetching is triggered. In this case we would remove
`ExceptionsConditions` component while loading data and re-create it
after. Since conditions state stored inside `ExceptionsConditions` we
will lose it.

This is a quick fix to make sure our users are not frustrated. The state
refactoring will come separately in the next release when we are going
to address the main issue
elastic/security-team#8197

To reproduce:
1. Open "add rule exception" flyout
2. Add exception conditions
3. Wait for 5 minutes (to avoid waiting this long you can use [this
approach](elastic#166550 (comment)))
4. Remove focus from the page (by switching to another app or navigating
to a different tab in a browser)
5. Focus on the page again

When you are back to the page all exception conditions should still be
there.

---------

Co-authored-by: Vitalii Dmyterko <[email protected]>
(cherry picked from commit a0631cf)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.12

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 12, 2023
@kibanamachine @e40pud
[8.12] [Security Solution] Exceptions flyout refreshes when it regain…
2ea119b 
…s focus, causing configured fields to get cleared (#166550) (#172666) (#173131)

# Backport

This will backport the following commits from `main` to `8.12`:
- [[Security Solution] Exceptions flyout refreshes when it regains
focus, causing configured fields to get cleared (#166550)
(#172666)](#172666)

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

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

<!--BACKPORT [{"author":{"name":"Ievgen
Sorokopud","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-12-12T09:14:57Z","message":"[Security
Solution] Exceptions flyout refreshes when it regains focus, causing
configured fields to get cleared (#166550) (#172666)\n\n##
Summary\r\n\r\nAddresses
https://github.com/elastic/kibana/issues/166550\r\n\r\nThese changes fix
the issue where user can experience data loss while\r\nadding rule
exceptions.\r\n\r\nThe main issue is that we keep conditions state
inside the child\r\ncomponent `ExceptionsConditions` which is rendered
conditionally based\r\non `isLoading` flag. This flag changes when
`useQuery` data gets stale\r\nand refetching is triggered. In this case
we would remove\r\n`ExceptionsConditions` component while loading data
and re-create it\r\nafter. Since conditions state stored inside
`ExceptionsConditions` we\r\nwill lose it.\r\n\r\nThis is a quick fix to
make sure our users are not frustrated. The state\r\nrefactoring will
come separately in the next release when we are going\r\nto address the
main
issue\r\nhttps://github.com/elastic/security-team/issues/8197\r\n\r\nTo
reproduce:\r\n1. Open \"add rule exception\" flyout\r\n2. Add exception
conditions\r\n3. Wait for 5 minutes (to avoid waiting this long you can
use
[this\r\napproach](https://github.com/elastic/kibana/issues/166550#issuecomment-1802941467))\r\n4.
Remove focus from the page (by switching to another app or
navigating\r\nto a different tab in a browser)\r\n5. Focus on the page
again\r\n\r\nWhen you are back to the page all exception conditions
should still be\r\nthere.\r\n\r\n---------\r\n\r\nCo-authored-by:
Vitalii Dmyterko
<[email protected]>","sha":"a0631cf8bd0ce820c1a3768f02e8384afc922b27","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:
SecuritySolution","backport:prev-minor","Team:Detection
Engine","v8.13.0"],"number":172666,"url":"https://github.com/elastic/kibana/pull/172666","mergeCommit":{"message":"[Security
Solution] Exceptions flyout refreshes when it regains focus, causing
configured fields to get cleared (#166550) (#172666)\n\n##
Summary\r\n\r\nAddresses
https://github.com/elastic/kibana/issues/166550\r\n\r\nThese changes fix
the issue where user can experience data loss while\r\nadding rule
exceptions.\r\n\r\nThe main issue is that we keep conditions state
inside the child\r\ncomponent `ExceptionsConditions` which is rendered
conditionally based\r\non `isLoading` flag. This flag changes when
`useQuery` data gets stale\r\nand refetching is triggered. In this case
we would remove\r\n`ExceptionsConditions` component while loading data
and re-create it\r\nafter. Since conditions state stored inside
`ExceptionsConditions` we\r\nwill lose it.\r\n\r\nThis is a quick fix to
make sure our users are not frustrated. The state\r\nrefactoring will
come separately in the next release when we are going\r\nto address the
main
issue\r\nhttps://github.com/elastic/security-team/issues/8197\r\n\r\nTo
reproduce:\r\n1. Open \"add rule exception\" flyout\r\n2. Add exception
conditions\r\n3. Wait for 5 minutes (to avoid waiting this long you can
use
[this\r\napproach](https://github.com/elastic/kibana/issues/166550#issuecomment-1802941467))\r\n4.
Remove focus from the page (by switching to another app or
navigating\r\nto a different tab in a browser)\r\n5. Focus on the page
again\r\n\r\nWhen you are back to the page all exception conditions
should still be\r\nthere.\r\n\r\n---------\r\n\r\nCo-authored-by:
Vitalii Dmyterko
<[email protected]>","sha":"a0631cf8bd0ce820c1a3768f02e8384afc922b27"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.13.0","labelRegex":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/172666","number":172666,"mergeCommit":{"message":"[Security
Solution] Exceptions flyout refreshes when it regains focus, causing
configured fields to get cleared (#166550) (#172666)\n\n##
Summary\r\n\r\nAddresses
https://github.com/elastic/kibana/issues/166550\r\n\r\nThese changes fix
the issue where user can experience data loss while\r\nadding rule
exceptions.\r\n\r\nThe main issue is that we keep conditions state
inside the child\r\ncomponent `ExceptionsConditions` which is rendered
conditionally based\r\non `isLoading` flag. This flag changes when
`useQuery` data gets stale\r\nand refetching is triggered. In this case
we would remove\r\n`ExceptionsConditions` component while loading data
and re-create it\r\nafter. Since conditions state stored inside
`ExceptionsConditions` we\r\nwill lose it.\r\n\r\nThis is a quick fix to
make sure our users are not frustrated. The state\r\nrefactoring will
come separately in the next release when we are going\r\nto address the
main
issue\r\nhttps://github.com/elastic/security-team/issues/8197\r\n\r\nTo
reproduce:\r\n1. Open \"add rule exception\" flyout\r\n2. Add exception
conditions\r\n3. Wait for 5 minutes (to avoid waiting this long you can
use
[this\r\napproach](https://github.com/elastic/kibana/issues/166550#issuecomment-1802941467))\r\n4.
Remove focus from the page (by switching to another app or
navigating\r\nto a different tab in a browser)\r\n5. Focus on the page
again\r\n\r\nWhen you are back to the page all exception conditions
should still be\r\nthere.\r\n\r\n---------\r\n\r\nCo-authored-by:
Vitalii Dmyterko
<[email protected]>","sha":"a0631cf8bd0ce820c1a3768f02e8384afc922b27"}}]}]
BACKPORT-->

Co-authored-by: Ievgen Sorokopud <[email protected]>
@e40pud e40pud deleted the security/bugfix/166550-exception-data-loss branch December 12, 2023 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitaliidm vitaliidm vitaliidm approved these changes

@yctercero yctercero Awaiting requested review from yctercero

Assignees

@e40pud e40pud

Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:fix Team:Detection Engine Security Solution Detection Engine Area Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.12.0 v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@e40pud @elasticmachine @kibana-ci @kibanamachine @vitaliidm