[Security Solution] [Flyout] drive flyout state with url or memory + support back button navigation from timelines by lgestc · Pull Request #169661 · elastic/kibana

lgestc · 2023-10-24T14:18:30Z

Summary

This is a PoC for flyout state (left, right, preview panels) stored entirely in the url without separate syncing mechanism. It is also possible to opt in for in-memory storage.

This vs current solution:

browser navigation is supported
we dont need to sync anything with in-memory state
we can remove useImperativeHandle from expandable flyout package
flyout state can be updated on the individual widget level, without prop drilling
when clicking between alerts, current flyout arrangement is retained - so the tabs you have open etc are still there (no custom code required)
it is now possible to investigate something in timeline using the flyout action & go back to the flyout view

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

packages/kbn-expandable-flyout/src/hooks/useUrlState.ts

x-pack/plugins/security_solution/public/common/hooks/timeline/use_init_timeline_url_param.ts

PhilippeOberti · 2023-11-14T20:19:39Z

I see some differences in behavior between flyout and timeline.

After closing the flyout, I can click on the browser back button and the flyout correctly reopens.

Screen.Recording.2023-11-14.at.2.14.56.PM.mov

But doing the same with timeline doesn't work. Timeline does not reopen on the screen.

Screen.Recording.2023-11-14.at.2.16.53.PM.mov

PhilippeOberti · 2023-11-14T20:22:59Z

There is also a weird behavior with timeline, where using the browser back button then going forward again reopens the timeline but looses the previously selected timetange, query and result (though the Query tab title has the correct result count)

Screen.Recording.2023-11-14.at.2.20.44.PM.mov

PhilippeOberti · 2023-11-14T20:37:59Z

This was probably not the original focus of this work, but I find it not user friendly that when I click on the bowser back button after having navigated through tabs within the flyout, we don't go back through these same tabs. It works for the very top level ones (Insights, Investigation and Response) but not for the children ones.
I can see 2 ways of improving this experience:

add the functionality to go through these
not push those type of changes to the browser history, so that going back would skip all these changes instead of going through them without any visual changes

Screen.Recording.2023-11-14.at.2.29.56.PM.mov

PhilippeOberti · 2023-11-14T20:40:27Z

The Share alert functionality is broken in this branch, when opening the new page the flyout doesn't open again. I'm guessing it could be related to the different key (experimentalFlyoutState) we're using in the url?

Screen.Recording.2023-11-14.at.2.38.30.PM.mov

PhilippeOberti · 2023-11-14T20:50:04Z

Opening a preview panel doesn't work anymore

Screen.Recording.2023-11-14.at.2.49.25.PM.mov

PhilippeOberti

In the current state of the PR, it seems that the reducer is almost not used anymore? Though we still returning the panels here, I'm not sure this is working anymore, and maybe it just need a bit more cleanup?

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

packages/kbn-expandable-flyout/src/hooks/root_key.ts

packages/kbn-expandable-flyout/src/hooks/use_left_panel.ts

packages/kbn-expandable-flyout/src/hooks/use_preview.ts

packages/kbn-expandable-flyout/src/hooks/use_url_state.ts

packages/kbn-expandable-flyout/index.ts

packages/kbn-expandable-flyout/src/hooks/use_left_panel.ts

lgestc · 2023-11-15T14:16:11Z

In the current state of the PR, it seems that the reducer is almost not used anymore? Though we still returning the panels here, I'm not sure this is working anymore, and maybe it just need a bit more cleanup?

hey, just removed it entirely

lgestc · 2023-11-15T14:17:33Z

This was probably not the original focus of this work, but I find it not user friendly that when I click on the bowser back button after having navigated through tabs within the flyout, we don't go back through these same tabs. It works for the very top level ones (Insights, Investigation and Response) but not for the children ones. I can see 2 ways of improving this experience:

add the functionality to go through these

not push those type of changes to the browser history, so that going back would skip all these changes instead of going through them without any visual changes

Screen.Recording.2023-11-14.at.2.29.56.PM.mov
working on putting these into the url as well! :) thanks

lgestc · 2023-11-15T14:58:47Z

Opening a preview panel doesn't work anymore

Screen.Recording.2023-11-14.at.2.49.25.PM.mov

should work now

packages/kbn-expandable-flyout/src/hooks/use_url_state.ts

PhilippeOberti

This is super close, left a few comments.

One thing @christineweng mentioned is that Kibana has a util package for url and memory management, @lgestc did you look into it by any chance? (I haven't yet)
src/plugins/kibana_utils/docs/state_containers/README.md
src/plugins/kibana_utils/docs/state_sync/README.md

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

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

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

x-pack/plugins/security_solution/public/flyout/document_details/left/tabs/visualize_tab.tsx

x-pack/plugins/security_solution/public/timelines/components/flyout/header/active_timelines.tsx

packages/kbn-expandable-flyout/README.md

lgestc · 2023-11-28T18:17:12Z

This is super close, left a few comments.

One thing @christineweng mentioned is that Kibana has a util package for url and memory management, @lgestc did you look into it by any chance? (I haven't yet) src/plugins/kibana_utils/docs/state_containers/README.md src/plugins/kibana_utils/docs/state_sync/README.md

why not, I will take a look.

PhilippeOberti · 2023-11-28T20:36:05Z

This is super close, left a few comments.
One thing @christineweng mentioned is that Kibana has a util package for url and memory management, @lgestc did you look into it by any chance? (I haven't yet) src/plugins/kibana_utils/docs/state_containers/README.md src/plugins/kibana_utils/docs/state_sync/README.md

why not, I will take a look.

@lgestc I debated adding the comment. I feel like the PR is so close and will be such a nice feature to have in 8.12 that I would consider fixing the few minor things left then merge it.
Changing to this Kibana package should be transparent to the developers and hopefully the way it works, so maybe something we can do later?

logeekal · 2023-11-29T10:21:22Z

packages/kbn-url-state/index.ts

@lgestc , love the simplicity of the API.

I understand that you are delaying updates to the next microtask with setTimeout but I don't 100% understand, how does it batches the state updates.

Also, I think something like debounce can provide a clearer api for batching very quick successive updates,

so the changes are first appended to some global object, and then flushed to the url, but we are going to drop this custom thing in favor of already made url storage solution linked somewhere here

logeekal

@PhilippeOberti , @lgestc

So I just reviewed kbn-url-state and so far it looks great. I have added couple of more questions/comments.

But I think there is something like it already exists. Sorry to say this so late in the party as I know you have put a lot of effort in this.

While working on Discover work, I observed that discover is already using url sync state management solution which is as shown below:

kibana/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts

Lines 59 to 70 in 6bdc094

    
           export const createKbnUrlStateStorage = ( 
        
             { 
        
               useHash = false, 
        
               useHashQuery = true, 
        
               history, 
        
               onGetError, 
        
               onSetError, 
        
             }: { 
        
               useHash: boolean; 
        
               useHashQuery?: boolean; 
        
               history?: History; 
        
               onGetError?: (error: Error) => void;

I am not sure if you are already aware of this. But do you think it makes sense to add your improvements in this existing plugin?

logeekal · 2023-11-29T10:25:50Z

packages/kbn-url-state/index.test.ts

Would be great to know the reason for removing these tests because this can surely use many more tests.

logeekal · 2023-11-29T10:36:26Z

packages/kbn-url-state/index.ts

Since, you are already implementing this, I would like to bring one more usecase for this to your attention.

In the Filter group controls on the Alerts Page, I use a custom hook to sync the url with the state as shown below.

kibana/x-pack/plugins/security_solution/public/common/components/filter_group/hooks/use_control_group_sync_to_local_storage.ts

Lines 13 to 23 in 6bdc094

interface UseControlGroupSyncToLocalStorageArgs {

storageKey: string;

shouldSync: boolean;

}

type UseControlGroupSyncToLocalStorage = (args: UseControlGroupSyncToLocalStorageArgs) => {

controlGroupInput: ControlGroupInput | undefined;

setControlGroupInput: Dispatch<SetStateAction<ControlGroupInput>>;

getStoredControlGroupInput: () => ControlGroupInput | undefined;

};

One requirement in this was to pause/resume sync on demand. For example, when user is editing the controls, we do not want to sync the state but otherwise, we want to.

Something to think about when finally implementing this.

I feel like this is out of scope for this PR. Would you be ok if we take a look at this later? Especially because we might actually drop this package entirely in favor for another already existing one...

100%, this was a use case for synching url changes that I wanted to bring to attention. No hurry on this.

and I agree on the point of using existing one. Thanks.

lgestc · 2023-11-29T11:52:46Z

This is super close, left a few comments.
One thing @christineweng mentioned is that Kibana has a util package for url and memory management, @lgestc did you look into it by any chance? (I haven't yet) src/plugins/kibana_utils/docs/state_containers/README.md src/plugins/kibana_utils/docs/state_sync/README.md

why not, I will take a look.

@lgestc I debated adding the comment. I feel like the PR is so close and will be such a nice feature to have in 8.12 that I would consider fixing the few minor things left then merge it. Changing to this Kibana package should be transparent to the developers and hopefully the way it works, so maybe something we can do later?

So, adding this to the flyout package results in the following error. I am yet to figure out what is the problem here.

michaelolo24 · 2023-11-29T14:41:48Z

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

silly nit, but can we just call this closeFlyout? I get the openPanels one since you can pass a configuration of panels, but this one just closes everything taking no args

I think keeping closePanels is ok here, as it was written like this originally, to make all the variable names consistent within the file. You have openPanels, openRightPanel, openLeftPanel, openPreviewPanel, closeRightPanel, closeLeftPanel, closePreviewPanel, and closePanels 😄

the developer facing API for the package showsopenFlyout and closeFlyout, which is what really matters.
I just have OCD wanted to have all the variable within that file be consistent...

PhilippeOberti

This is really awesome, thank you for going through and implementing this! The code looks perfect to me 😃
I tested a lot of scenarios and couldn't find any issues!

PhilippeOberti · 2023-11-29T15:24:31Z

packages/kbn-expandable-flyout/src/hooks/use_left_panel.ts

if you make another change to the PR and have to push some code, let's remove this @returns

…fix'

lgestc · 2023-11-29T17:39:23Z

This is really awesome, thank you for going through and implementing this! The code looks perfect to me 😃 I tested a lot of scenarios and couldn't find any issues!

<3

kibana-ci · 2023-11-29T19:00:54Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4733	4737	+4

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`	14	7	-7

Async chunks

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

id	before	after	diff
`securitySolution`	12.9MB	12.9MB	+33.2KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/expandable-flyout`	3	2	-1

Unknown metric groups

API count

id	before	after	diff
`@kbn/expandable-flyout`	36	17	-19
`@kbn/url-state`	4	3	-1
total			-20

ESLint disabled line counts

id	before	after	diff
`securitySolution`	464	463	-1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	534	533	-1

History

💚 Build #179965 succeeded 4dafa7b923efa110e1e8bb89ed094f97cd436978
💛 Build #179559 was flaky 2dc10dc60959d27d7faffa06d8f05ba73baac529
💚 Build #179460 succeeded 1191bd6084c166d2a248dafb828d10fc1b5b5660
💔 Build #179420 failed 7b14c39d69ff0e7fc2ddf18d8169a060bde993f1
💔 Build #179386 failed 091e4f5f7fb9c1f37e4d5876b60a815f5b8b1374
💔 Build #179243 failed 1171b0e6600f2ffbbc52a4810aaacc1a6d3bcd4b

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

rylnd

Reviewed on behalf of detection-engine and entity-analytics. Code changes there look pretty innocuous, all the interesting stuff is on the other teams 😉 .

I had one question about a comment's utility, but generally this is fine from our end. LGTM.

rylnd · 2023-11-29T19:18:30Z

...ns/security_solution/public/flyout/entity_details/shared/components/risk_summary.stories.tsx

  openLeftPanel: () => window.alert('openLeftPanel called'),
  panels: {},
-} as unknown as ExpandableFlyoutContext;
+} as unknown as ExpandableFlyoutContextValue;


Good catch here 👍

rylnd · 2023-11-29T19:20:46Z

x-pack/plugins/security_solution/public/flyout/index.tsx

-  );
-};
-
+// NOTE: provider below accepts "storage" prop, please take a look into component's JSDoc.


I'm not sure this comment is doing much. Are you intending to indicate that they may change the component's behavior by changing this hardcoded prop? Why is that important here?

lgestc requested review from PhilippeOberti and michaelolo24 October 24, 2023 14:18

michaelolo24 reviewed Oct 27, 2023

View reviewed changes

packages/kbn-expandable-flyout/src/hooks/useUrlState.ts Outdated Show resolved Hide resolved

michaelolo24 reviewed Oct 27, 2023

View reviewed changes

packages/kbn-expandable-flyout/src/hooks/useUrlState.ts Outdated Show resolved Hide resolved

lgestc force-pushed the flyout_state_exclusively_in_url branch from a962059 to b652e59 Compare November 7, 2023 17:05

lgestc force-pushed the flyout_state_exclusively_in_url branch 2 times, most recently from 6c417ba to 4ca42f5 Compare November 14, 2023 17:53

lgestc commented Nov 14, 2023

View reviewed changes

x-pack/plugins/security_solution/public/common/hooks/timeline/use_init_timeline_url_param.ts Outdated Show resolved Hide resolved

PhilippeOberti reviewed Nov 14, 2023

View reviewed changes

lgestc force-pushed the flyout_state_exclusively_in_url branch 3 times, most recently from 341c624 to d87b687 Compare November 15, 2023 14:13

lgestc force-pushed the flyout_state_exclusively_in_url branch 2 times, most recently from dccc38a to b4a223b Compare November 15, 2023 14:57

lgestc force-pushed the flyout_state_exclusively_in_url branch 3 times, most recently from d81820f to e072101 Compare November 16, 2023 16:30

kqualters-elastic reviewed Nov 16, 2023

View reviewed changes

packages/kbn-expandable-flyout/src/hooks/use_url_state.ts Outdated Show resolved Hide resolved

christineweng mentioned this pull request Nov 16, 2023

[Security Solution] Use new expandable flyout in alert preview #167902

Merged

2 tasks

lgestc force-pushed the flyout_state_exclusively_in_url branch 3 times, most recently from 4871114 to c6a8fdb Compare November 20, 2023 21:13

lgestc requested review from a team as code owners November 28, 2023 13:52

lgestc requested review from PhilippeOberti, michaelolo24 and rylnd November 28, 2023 13:52

lgestc added 8.12 candidate v8.12.0 release_note:enhancement labels Nov 28, 2023

PhilippeOberti reviewed Nov 28, 2023

View reviewed changes

logeekal reviewed Nov 29, 2023

View reviewed changes

lgestc force-pushed the flyout_state_exclusively_in_url branch 3 times, most recently from 195803a to 4edce9f Compare November 29, 2023 11:51

lgestc changed the title ~~drive flyout state with url or memory + support back button navigation from timelines~~ [Security Solution] [Flyout] drive flyout state with url or memory + support back button navigation from timelines Nov 29, 2023

lgestc added Feature:Expandable Flyout Expandable flyout used in the security solution Team:Threat Hunting:Investigations Security Solution Threat Hunting Investigations Team labels Nov 29, 2023

michaelolo24 reviewed Nov 29, 2023

View reviewed changes

PhilippeOberti approved these changes Nov 29, 2023

View reviewed changes

lgestc and others added 2 commits November 29, 2023 18:23

flyout state done with url only

dccafa1

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

13b81c5

…fix'

rylnd approved these changes Nov 29, 2023

View reviewed changes

PhilippeOberti mentioned this pull request Dec 27, 2023

[Security Solution][Timeline] fix share alert not working with alert details expandable flyout #174005

Merged

	export const createKbnUrlStateStorage = (
	{
	useHash = false,
	useHashQuery = true,
	history,
	onGetError,
	onSetError,
	}: {
	useHash: boolean;
	useHashQuery?: boolean;
	history?: History;
	onGetError?: (error: Error) => void;

	interface UseControlGroupSyncToLocalStorageArgs {
	storageKey: string;
	shouldSync: boolean;
	}

	type UseControlGroupSyncToLocalStorage = (args: UseControlGroupSyncToLocalStorageArgs) => {
	controlGroupInput: ControlGroupInput \| undefined;
	setControlGroupInput: Dispatch<SetStateAction<ControlGroupInput>>;
	getStoredControlGroupInput: () => ControlGroupInput \| undefined;
	};

Conversation

lgestc commented Oct 24, 2023 • edited by PhilippeOberti Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

This vs current solution:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PhilippeOberti commented Nov 14, 2023

Uh oh!

PhilippeOberti commented Nov 14, 2023

Uh oh!

PhilippeOberti commented Nov 14, 2023

Uh oh!

PhilippeOberti commented Nov 14, 2023

Uh oh!

PhilippeOberti commented Nov 14, 2023

Uh oh!

PhilippeOberti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgestc commented Nov 15, 2023

Uh oh!

lgestc commented Nov 15, 2023

Uh oh!

lgestc commented Nov 15, 2023

Uh oh!

Uh oh!

PhilippeOberti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgestc commented Nov 28, 2023

Uh oh!

PhilippeOberti commented Nov 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logeekal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgestc commented Nov 29, 2023

Uh oh!

michaelolo24 Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhilippeOberti left a comment

lgestc commented Oct 24, 2023 •

edited by PhilippeOberti

Loading

logeekal left a comment •

edited

Loading

michaelolo24 Nov 29, 2023 •

edited

Loading