[Security Solution][Expandable flyout] add multi flyout support to kbn-expandable-flyout package #176457

PhilippeOberti · 2024-02-07T23:35:06Z

Summary

This PR primarily makes a change to the kbn-expandable-flyout. The goal is to allow multiple expandable flyout to be rendered at the same time (our use case right now is to have a flyout for the alerts page and another flyout for timeline).

Notes: for the sake of keeping the PR small, it does NOT implement multiple flyout in the Security Solution codebase. This will come in a follow up PR.

These are the changes:

introduce a new urlKey props that developer have to pass when wanting to use the expandable flyout. This will be used as a url parameter to save the state of the flyout in the url. I also removed the storage props as it is now redundant
with the props changed, I reverted the default storage method. The flyout is now by default stored in memory, and when developers pass an urlKey the storage is done in the url. This makes more sense as the urlKey would be necessary in that case
update the redux store to be an object { [urlKey: string] : {left, right, preview} }, so we can store multiple flyouts at the same time
sets the default value for preview panels to undefined instead of [] to make the url look a bit cleaner when we close preview panels or the entire flyout (previously we still have something like preview:() after closing)

No UI changes should be visible.

A follow up PR will focus on updating the code in the Security Solution plugin, to focus on having multiple flyouts.

What to test

verify the expandable flyout shows up correctly in the few pages we're using it today:
- alerts page
- cases page
- rule preview
- explore pages
refreshing the page should reopen the flyout with the same panels open
verify that copying the url or clicking on the share alerts button correctly reopens the flyout with the same panels open

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

https://github.com/elastic/security-team/issues/7464

elasticmachine · 2024-02-07T23:35:08Z

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

packages/kbn-expandable-flyout/src/provider.tsx

kqualters-elastic · 2024-02-09T07:31:27Z

i'm confused why we have the url key get reflected in state, wouldn't you either have 1 url key per provider, thus defeating the purpose because each flyout has it's own provider/redux store, or all flyouts using the same url key with 1 store would overwrite each others state? seems like it would maybe make more sense to have the url_key be a prop of the component, and only 1 provider is ever in the DOM at a time no?

logeekal

Desk tested this PR. Works great 🚀 . I tested back and forward buttons and well and flyout is working perfectly. Only one observation I have is when flyout is closed, eventFlyout key remains in the url as undefined ( eventFlyout=()).

I understand that is why you were asking about deleting a search param.

Apart from desk testing, I had below suggestions and questions.

packages/kbn-expandable-flyout/src/context.tsx

packages/kbn-expandable-flyout/src/actions.ts

x-pack/plugins/security_solution/public/app/home/template_wrapper/index.tsx

packages/kbn-expandable-flyout/src/constants.ts

logeekal · 2024-02-09T10:23:00Z

packages/kbn-expandable-flyout/src/index.test.tsx

@@ -45,12 +45,16 @@ describe('ExpandableFlyout', () => {

  it('should render right section', () => {


Could you please add unit tests suite which simulate opening/closing of single/multiple flyouts ( separately and together) and assert corresponding storage/url changes.

I think the package tests are very important to make sure it is handling all the cases consumers are going to use it for.

I'm not sure I understand here. There should always be a single flyout per provider (as mentioned in the README) so what other tests could I add here?

Okay i understand. Initially, I was taken aback with the design of having one provider per flyout, since I assumed from the redux state that there will be multiple flyout handled by one provider. But in your other comment you have stated that end goal is to have one provider for all flyouts, I think then these tests will be required. For this PR, this comment can be ignored.

Thanks

Now I'm even more confused! There is currently a single instance of a flyout per expandable flyout provider and this is not going to change. If you're talking about this comment, I said the goal it NOT to have one provider only.
I feel like we might be talking about a different provider level (expandable flyout provider vs internal context provider vs redux provider). To be super clear, this is what we should have:

<ExpandableFlyoutProvider> <SingleFlyout /> </ ExpandableFlyoutProvider>

I see that I could add test to check that memory mode doesn't store anything in the url and that url mode indeed stores things in the url.
Though testing multiple providers for multiple flyouts seem more some UI integration test than unit tests no? Can I watch the changes to the url if I'm only rendering a single component in Jest?

logeekal · 2024-02-09T10:25:04Z

packages/kbn-expandable-flyout/src/provider.tsx

 import { urlChangedAction } from './actions';

-export type ExpandableFlyoutStorageMode = 'memory' | 'url';
-
 /**
 * Dispatches actions when url state changes and initializes the state when the app is loaded with flyout url parameters
 */
 const UrlSynchronizer = () => {


nit: Since this component is not rendering anything. Do you think it can only be a hook and taken out of this file completely?

Additionaly, there seem to be no test for this file. Could you please add those?

I don't think we can change this to a hook because we need to conditionally render it (look line 96 of the file).
I can extract it though and write some tests for it!

I finally decided against extracting it, but I have added unit tests

I don't think we can change this to a hook because we need to conditionally render it (look line 96 of the file).

I think that is more of a design question. It can simply be made into a hook by passing needSync : boolean as a parameter to useUrlSync and then run effects based on that. I still think that it is more suitable as hook.

But It is only nit from my side so please go ahead with what you think is good. Thanks for making other changes.

I'm still not sure how I would be able to do this. Let me ping on on Slack

packages/kbn-expandable-flyout/src/provider.tsx

christineweng · 2024-02-12T21:07:17Z

packages/kbn-expandable-flyout/src/state.ts

@@ -20,8 +20,16 @@ export interface State {
  /**
   * Panels to render in the preview section
   */
-  preview: FlyoutPanelProps[];
+  preview: FlyoutPanelProps[] | undefined;


as discussed, now that we are storing state in redux, consider updating the type of FlyoutPanelProps to be serializable (line)

export interface FlyoutPanelProps { ... /** * Any parameters necessary for the initial requests within the flyout */ params?: Record<string, unknown>;

yup I'm going to have to think about how to do that, I'll do it in a follow up PR!

christineweng · 2024-02-12T21:08:38Z

x-pack/plugins/security_solution/public/common/hooks/use_url_state.ts

@@ -16,6 +15,8 @@ import { useQueryTimelineByIdOnUrlChange } from './timeline/use_query_timeline_b
 import { useInitFlyoutFromUrlParam } from './flyout/use_init_flyout_url_param';
 import { useSyncFlyoutUrlParam } from './flyout/use_sync_flyout_url_param';

+export const EXPANDABLE_FLYOUT_URL_KEY = 'eventFlyout' as const;


maybe detailsFlyout is more suitable?

yes, I'll leave that to the next PR though, this one is just about adding support to multiple flyouts

logeekal · 2024-02-13T09:06:28Z

packages/kbn-expandable-flyout/src/test/provider.tsx

@@ -9,17 +9,20 @@
 import { Provider as ReduxProvider } from 'react-redux';
 import { configureStore } from '@reduxjs/toolkit';
 import React, { FC, PropsWithChildren } from 'react';


@PhilippeOberti , I would recommend renaming folder to mock for clarity.

logeekal · 2024-02-13T09:13:04Z

packages/kbn-expandable-flyout/src/provider.test.tsx

+      preview: undefined,
+    });
+    expect(mockSet).toHaveBeenCalled();
+  });


Do you think it should have one more use case where if url is changed, then it should also trigger redux state change?

For example, if a url visits are page with &urlKey=(...), it should modify redux state to expand the flyout.

logeekal

Thank you for making all the needed changes. LGTM 🚀

Approving with couple of very small comments/suggestions. Thank you.

…ackage to support multiple flyout opened at the same time

kibana-ci · 2024-02-15T23:13:25Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: cf0dc68

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #3 / CustomFields renders correctly

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	5001	5002	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/expandable-flyout`	15	14	-1

Async chunks

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

id	before	after	diff
`securitySolution`	11.6MB	11.6MB	+1.6KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/expandable-flyout`	39	37	-2

History

💔 Build #193708 failed ef874b30d1413a5df1544c7f0670a07a9589994e
💔 Build #193698 failed c381eb4c479f17ea90986a4ebc0cefcc006ae472
💔 Build #193682 failed 1669bbb61eeffe798ce1441f8247a50d4e9f4b4e
💚 Build #192671 succeeded 1a11d7ce8e4e0851d5946564e31b4a6c12a37fa6
💛 Build #192444 was flaky 60055ac8dc0df270a3c286008719754e86e2fd03

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

…n-expandable-flyout package (elastic#176457) (cherry picked from commit a2cb03b)

kibanamachine · 2024-02-15T23:18:18Z

💚 All backports created successfully

Status	Branch	Result
✅	8.13

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

Questions ?

Please refer to the Backport tool documentation

…n-expandable-flyout package (elastic#176457)

## Summary This PR enables Timeline to have its own expandable flyout, independent from the Security Solution app level flyout. This previous [PR](#176457) added support to the `kbn-expandable-flyout` package to be able to show multiple flyout at the same time. This PR focuses on: - changing the location where we use the `<ExpandableFlyoutProvider>` to wrap only the Security Solution pages. instead of the entire Security Solution application - wrapping `Timeline` with its own `<ExpandableFlyoutProvider>` - opening the new expandable flyout from timeline for alerts, users and host!! ### Notes - the new expandable flyout opened from `Timeline` is saved in the url under a new `timelineFlyout` parameter. The other Security Solution flyout stays saved under `eventFlyout` so both can be displayed at the same time - introduced a new feature flag called `expandableTimelineFlyoutEnabled`, enabled to `false` by default. The code uses this flag in combination with the already existing `securitySolution:enableExpandableFlyout` advanced settings to toggle on/off the expandable flyout in `Timeline` I had to make a small change to the timeline `z-index` value, from its hardcoded value of `1000` to now `1001`. At the same time I'm setting the `z-index` of the `eventFlyout` to `1000` and the `z-index` of the `timelineFlyout` to `1002`. Finally, I changed the `z-index` of all the other flyouts opened from the `Take action` button in the footer (like _Add to new case_, _Add rule exception_...) to `1003`. This was the only way to get the order correct after page refresh, and also work in Serverless which has a different top level layout (for the top bar and the left navigation panel). ### Testing Make sure you have the flag turned on in your `kibana.yml` file: `xpack.securitySolution.enableExperimental: ['expandableTimelineFlyoutEnabled']` While the code changes are pretty minimal, emphasize should be done on testing: - verify that the non-timeline expandable flyout behavior hasn't changed (in the Explore, Rule Preview, Alerts pages...) - verify the advanced settings correctly toggles on and off the expandable flyout for both the Security Solution app and Timeline - verify the expandable flyout correctly shows up in Timeline on all the pages where Timeline is present - verify both flyout can be shown at the same time - verify refreshing the page reloads correctly all flyouts open - verify copying the url or using the Share Alert button at the top can be correctly reopened in another tab ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### TODO - [x] fix unit tests - [ ] fix e2e tests - [x] figure out how to fix the UI in serverless (the flyout is currently behind the left navigation) Before change: https://github.com/elastic/kibana/assets/17276605/3b89f87f-d1fa-4635-8ff7-855795eb3796 After change: https://github.com/elastic/kibana/assets/17276605/6b341f11-054c-4885-b496-006f37b8d572 Serverless https://github.com/elastic/kibana/assets/17276605/df414140-31df-4941-869e-c177cfedd805 elastic/security-team#7464 Details of file changed in [the comment below](#177087 (comment))

PhilippeOberti added release_note:enhancement Team:Threat Hunting:Investigations Security Solution Investigations Team v8.13.0 labels Feb 7, 2024

PhilippeOberti requested review from a team as code owners February 7, 2024 23:35

PhilippeOberti requested review from lgestc and christineweng February 7, 2024 23:35

PhilippeOberti commented Feb 7, 2024

View reviewed changes

packages/kbn-expandable-flyout/src/provider.tsx Outdated Show resolved Hide resolved

PhilippeOberti force-pushed the support-multiple-expandable-flyout branch 3 times, most recently from c26b8eb to 81d0d75 Compare February 8, 2024 14:14

lgestc approved these changes Feb 8, 2024

View reviewed changes

PhilippeOberti force-pushed the support-multiple-expandable-flyout branch 5 times, most recently from 1bdcee4 to 60055ac Compare February 9, 2024 02:10

logeekal reviewed Feb 9, 2024

View reviewed changes

PhilippeOberti force-pushed the support-multiple-expandable-flyout branch from 60055ac to 1a11d7c Compare February 9, 2024 18:35

PhilippeOberti requested a review from logeekal February 12, 2024 15:18

christineweng reviewed Feb 12, 2024

View reviewed changes

logeekal reviewed Feb 13, 2024

View reviewed changes

logeekal approved these changes Feb 13, 2024

View reviewed changes

PhilippeOberti force-pushed the support-multiple-expandable-flyout branch 3 times, most recently from c381eb4 to ef874b3 Compare February 15, 2024 00:02

[Security Solution][Expandable flyout] update kbn-expandable-flyout p…

4f5b61c

…ackage to support multiple flyout opened at the same time

PR comments

cf0dc68

PhilippeOberti force-pushed the support-multiple-expandable-flyout branch from ef874b3 to cf0dc68 Compare February 15, 2024 21:36

PhilippeOberti merged commit a2cb03b into elastic:main Feb 15, 2024
36 checks passed

PhilippeOberti removed release_note:enhancement v8.13.0 labels Feb 15, 2024

kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Feb 15, 2024

PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes and removed backport:skip This commit does not require backporting v8.14.0 labels Feb 15, 2024

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 15, 2024

[Security Solution][Expandable flyout] add multi flyout support to kb…

8acb3a1

…n-expandable-flyout package (elastic#176457) (cherry picked from commit a2cb03b)

kibanamachine mentioned this pull request Feb 15, 2024

[8.13] [Security Solution][Expandable flyout] add multi flyout support to kbn-expandable-flyout package (#176457) #177099

Closed

PhilippeOberti mentioned this pull request Feb 15, 2024

[Security Solution] - use expandable flyout in timeline #177087

Merged

4 tasks

jbudz added backport:skip This commit does not require backporting v8.14.0 labels Feb 15, 2024

fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024

[Security Solution][Expandable flyout] add multi flyout support to kb…

6e17a8d

…n-expandable-flyout package (elastic#176457)

PhilippeOberti deleted the support-multiple-expandable-flyout branch November 14, 2024 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Expandable flyout] add multi flyout support to kbn-expandable-flyout package #176457

[Security Solution][Expandable flyout] add multi flyout support to kbn-expandable-flyout package #176457

PhilippeOberti commented Feb 7, 2024 •

edited

Loading

elasticmachine commented Feb 7, 2024

kqualters-elastic commented Feb 9, 2024

logeekal left a comment •

edited

Loading

logeekal Feb 9, 2024

PhilippeOberti Feb 9, 2024

logeekal Feb 13, 2024 •

edited

Loading

PhilippeOberti Feb 13, 2024

logeekal Feb 9, 2024

logeekal Feb 9, 2024 •

edited

Loading

PhilippeOberti Feb 9, 2024

PhilippeOberti Feb 9, 2024

logeekal Feb 13, 2024

PhilippeOberti Feb 13, 2024

christineweng Feb 12, 2024

PhilippeOberti Feb 13, 2024

christineweng Feb 12, 2024

PhilippeOberti Feb 13, 2024

logeekal Feb 13, 2024

logeekal Feb 13, 2024

logeekal left a comment •

edited

Loading

kibana-ci commented Feb 15, 2024

API count

kibanamachine commented Feb 15, 2024

		@@ -45,12 +45,16 @@ describe('ExpandableFlyout', () => {

		it('should render right section', () => {

[Security Solution][Expandable flyout] add multi flyout support to kbn-expandable-flyout package #176457

[Security Solution][Expandable flyout] add multi flyout support to kbn-expandable-flyout package #176457

Conversation

PhilippeOberti commented Feb 7, 2024 • edited Loading

Summary

What to test

Checklist

elasticmachine commented Feb 7, 2024

kqualters-elastic commented Feb 9, 2024

logeekal left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logeekal Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logeekal Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logeekal left a comment • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Feb 15, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

API count

History

kibanamachine commented Feb 15, 2024

💚 All backports created successfully

Questions ?

PhilippeOberti commented Feb 7, 2024 •

edited

Loading

logeekal left a comment •

edited

Loading

logeekal Feb 13, 2024 •

edited

Loading

logeekal Feb 9, 2024 •

edited

Loading

logeekal left a comment •

edited

Loading