[SecuritySolution] Close field editor when page context is lost #124378

semd · 2022-02-02T16:59:17Z

Summary

Implemented the close action on the field editor, by leveraging the returned function from the dataViewFieldEditor.openEditor function call.

The reference is needed here because the button that triggers the field editor flyout is unmounted when it is clicked. The closeEditor action needs to be triggered when the original component that opened the "fieldBrowser" in the first place is unmounted (alerts tables) or it is hidden (timeline):

close_field_editor_flyout_fix.mov

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2022-02-02T16:59:20Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

machadoum

The code looks great. I just noticed one edge case. Because timeline only closed the field editor when show value changes. It doesn't close the editor when you navigate to a security solutions page that doesn't have a timeline (Endpoint page).

...ns/security_solution/public/timelines/components/timeline/body/column_headers/index.test.tsx

semd · 2022-02-03T14:35:13Z

Hi @machadoum , very good catches! I addressed your suggestions and I improved the tests a little bit more. Thanks!

machadoum

LGTM

stephmilovic · 2022-02-03T15:47:31Z

x-pack/plugins/security_solution/public/common/components/events_viewer/index.tsx

@@ -137,6 +142,10 @@ const StatefulEventsViewerComponent: React.FC<Props> = ({
    }
    return () => {
      deleteEventQuery({ id, inputId: 'global' });
+      if (editorActionsRef.current) {
+        // eslint-disable-next-line react-hooks/exhaustive-deps


do you need this here? i thought just above the dependency array

yes, It is needed here too, this error pops if not diabled:

The ref value 'fieldEditorActionsRef.current' will likely have changed by the time this effect cleanup function runs. If this ref points to a node rendered by React, copy 'fieldEditorActionsRef.current' to a variable inside the effect, and use that variable in the cleanup function

stephmilovic · 2022-02-03T15:48:26Z

...ns/security_solution/public/timelines/components/timeline/body/column_headers/index.test.tsx

+      sortDirection: Direction.desc,
+    },
+  ];
+  const defaultProps: ColumnHeadersComponentProps = {


thanks for cleaning this up!

stephmilovic

just a question about that type exception. tested locally and LGTM. thanks @semd !

kibana-ci · 2022-02-03T16:20:25Z

💚 Build Succeeded

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`	4.7MB	4.7MB	+426.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	447	449	+2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	515	517	+2

History

💛 Build #21434 was flaky e25c441
💔 Build #21422 failed 7bf8aba

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

cc @semd

kibanamachine · 2022-02-07T17:06:19Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124378 or prevent reminders by adding the backport:skip label.

…tic#124378) * close field editor when context is lost * tests added * typecheck clean * close editor when timeline is unmounted (cherry picked from commit 21710df)

…tic#124378) * close field editor when context is lost * tests added * typecheck clean * close editor when timeline is unmounted (cherry picked from commit 21710df) # Conflicts: # x-pack/plugins/security_solution/public/timelines/components/create_field_button/index.tsx

) (#124934) * close field editor when context is lost * tests added * typecheck clean * close editor when timeline is unmounted (cherry picked from commit 21710df)

kibanamachine · 2022-02-08T17:16:20Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

kibanamachine · 2022-02-09T17:26:19Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

kibanamachine · 2022-02-10T17:36:21Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

…#124378) (#124935) * [SecuritySolution] Close field editor when page context is lost (#124378) * close field editor when context is lost * tests added * typecheck clean * close editor when timeline is unmounted (cherry picked from commit 21710df) # Conflicts: # x-pack/plugins/security_solution/public/timelines/components/create_field_button/index.tsx * fix test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

close field editor when context is lost

2f4ba1b

semd added bug Fixes for quality problems that affect the customer experience v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team v8.1.0 Team:Threat Hunting:Explore labels Feb 2, 2022

semd requested a review from a team as a code owner February 2, 2022 16:59

semd self-assigned this Feb 2, 2022

semd added 2 commits February 2, 2022 19:00

tests added

7bf8aba

typecheck clean

e25c441

machadoum requested changes Feb 3, 2022

View reviewed changes

...ns/security_solution/public/timelines/components/timeline/body/column_headers/index.test.tsx Outdated Show resolved Hide resolved

close editor when timeline is unmounted

1c071a5

machadoum approved these changes Feb 3, 2022

View reviewed changes

stephmilovic reviewed Feb 3, 2022

View reviewed changes

stephmilovic approved these changes Feb 3, 2022

View reviewed changes

semd merged commit 21710df into elastic:main Feb 3, 2022

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 7, 2022

semd mentioned this pull request Feb 8, 2022

[8.1] [SecuritySolution] Close field editor when page context is lost (#124378) #124934

Merged

semd mentioned this pull request Feb 8, 2022

[8.0] [SecuritySolution] Close field editor when page context is lost (#124378) #124935

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecuritySolution] Close field editor when page context is lost #124378

[SecuritySolution] Close field editor when page context is lost #124378

semd commented Feb 2, 2022

elasticmachine commented Feb 2, 2022

machadoum left a comment

semd commented Feb 3, 2022

machadoum left a comment

stephmilovic Feb 3, 2022

semd Feb 3, 2022

stephmilovic Feb 3, 2022

stephmilovic left a comment

kibana-ci commented Feb 3, 2022

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Feb 7, 2022

kibanamachine commented Feb 8, 2022

kibanamachine commented Feb 9, 2022

kibanamachine commented Feb 10, 2022

[SecuritySolution] Close field editor when page context is lost #124378

[SecuritySolution] Close field editor when page context is lost #124378

Conversation

semd commented Feb 2, 2022

Summary

Checklist

elasticmachine commented Feb 2, 2022

machadoum left a comment

Choose a reason for hiding this comment

semd commented Feb 3, 2022

machadoum left a comment

Choose a reason for hiding this comment

stephmilovic Feb 3, 2022

Choose a reason for hiding this comment

semd Feb 3, 2022

Choose a reason for hiding this comment

stephmilovic Feb 3, 2022

Choose a reason for hiding this comment

stephmilovic left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 3, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Feb 7, 2022

kibanamachine commented Feb 8, 2022

kibanamachine commented Feb 9, 2022

kibanamachine commented Feb 10, 2022