[Security Solution][Details Flyout] Fix multi-preview url sync #186130

christineweng · 2024-06-12T20:09:13Z

Summary

This PR fixed a preview rendering bug and made some improvements to the preview navigation:

If a preview is already opened, opening another preview should work properly. urlChangedAction is now dispatching the last item in preview array.
Added checks to not append a preview if it was last added
When state is stored in url (everywhere except rule preview), Back action in preview utilizes the browser go back functionality. This allows full synchronization between redux and url state.
Keep the latest preview in url to avoid url length explosion

Note: in order to make the interaction smooth, the Back button is now always present. When there is one preview open, clicking Go back will close the preview.

How to test

Enable feature flag entityAlertPreviewEnabled
Generate some alerts, go to Alerts Page
Expand detail on an alert, expand details, entities, clicking the host and user names

Screen.Recording.2024-06-27.at.4.43.54.PM.mov

elasticmachine · 2024-06-12T20:10:08Z

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

christineweng · 2024-06-12T20:10:35Z

...lugins/security_solution/public/flyout/document_details/left/components/entities_details.tsx

for demo purposes - to be removed before merging

PhilippeOberti

I think there is more to this that we need to look at. Let's sync when we're both working again (I think Monday June 24th)

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

PhilippeOberti · 2024-06-17T22:44:17Z

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

so I ended up spending 2 hours trying to debug this. I couldn't remember why we have the first dispatch line 49 and the second one within the subscribe line 59.

After a lot of debugging I understand that the first one is used to reopen the flyout after a page refresh. By the way, we should correct the preview: currentValue?.preview?.[0], line 52 and change it to preview: currentValue?.preview?.at(-1), or the preview that opens is the first one in the array.
While do are ok with loosing the history of previews after a refresh, it makes more sense to reopen the last preview after refresh as the UI will look similar.

As for the second second one (the one within the subscribe) I do not understand what it's doing. If I comment the code, and I'm not seeing anything weird happening... everything seems to still be working!

One last thing, when looking at the second useEffect in this file, I think we might have another problem...
This line secifically urlStorage.set(urlKey, { left, right, preview }); the preview variable here can be an array of multiple values, and I'm wondering if that's what's causing some of the weirdness. I wonder if we could always send an array of a single value (like urlStorage.set(urlKey, { left, right, preview: [preview?.at(-1)] });) or even just a plain object (no array, like so urlStorage.set(urlKey, { left, right, preview: preview?.at(-1) });).
Doing so we would loose the history of previews after refresh, but we don't care about that anyway...
We might have to tweak the code from the previous comment to preview: currentValue?.preview line 52 and I think this would work!

After more testing, there is another scenario that is not working:

Open user preview

Open a host preview

Refresh page: page shows host preview (which is correct)

Click Go Back: this is where it starts breaking: the urlSync action sends the user preview, but we always appends the array if the preview is not identical to the last, in this case, we should have removed. You can see in the video, after clicking go back in browsers, it actually adds the user preview

Screen.Recording.2024-06-24.at.9.53.12.PM.mov

I think if we were to get the synchronization work: both url and redux have to have the correct states. i pushed another commit where I passed the entire preview array to redux, just for discussion. Let me know what you think!

PhilippeOberti

So I did some more testing today with the newest code and I think this is very close.

The only thing I found was kind of a weird behavior is if you have multiple previews and click on the back button from the flyout, then using the back button from the browser basically undoes the back action from the flyout and adds back the last preview that was just removed.
I think this is logical when looking at the code, as the back button from the flyout doesn't leverage the back button from the browser.

Maybe we consider that is an ok behavior for now, until we refactor this back behavior better. What you have is already a huge improvement and fixes a bug that will be soon highlighted by the entity and alert preview PRs...

Let's chat about it later!

christineweng · 2024-06-27T21:02:12Z

@PhilippeOberti after some brainstorming and testing, I reverted the preview as array approach. I think the new approach is better and avoids url explosion. Let me know what you think!

christineweng · 2024-06-27T23:54:15Z

/ci

kibana-ci · 2024-06-28T01:26:20Z

💚 Build Succeeded

Buildkite Build
Commit: 8a87162

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.5MB	13.5MB	+4.8KB

History

💔 Build #218332 failed f1d3976
💔 Build #217566 failed 2b8859a

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

cc @christineweng

PhilippeOberti · 2024-06-28T11:47:38Z

I think a bug might have been introduced with your latest change, the Back button in the top left corner of the preview section always appears, even on the very first preview opened

Screen.Recording.2024-06-28.at.1.46.21.PM.mov

PhilippeOberti · 2024-06-28T12:10:28Z

Also with this new change, I'm wondering why we keep the preview slice of redux as an array. As you can see on the video below, we always have a single entry (though temporarily none or 2...)

Screen.Recording.2024-06-28.at.2.07.22.PM.mov

christineweng · 2024-06-28T15:42:10Z

Also with this new change, I'm wondering why we keep the preview slice of redux as an array. As you can see on the video below, we always have a single entry (though temporarily none or 2...)

Keeping the preview as array because of in memory mode. When in alert preview, we are relying on redux to manage state..

Another point being, the url store is based on the FlyoutState, where preview is an array. I wonder if we change the data structure it might affect urls generated pre-8.15?

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

Line 58 in 061a5ef

    
           const subscription = urlStorage.change$<FlyoutState>(urlKey).subscribe((value) => {

christineweng · 2024-06-28T15:49:02Z

I think a bug might have been introduced with your latest change, the Back button in the top left corner of the preview section always appears, even on the very first preview opened

Yes, this is intended. PR summary now reflect the change:

Note: in order to make the interaction smooth, the Back button is now always present. When there is one preview open, clicking Go back will close the preview.

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

Line 73 in 061a5ef

const showBackButton = !!preview && preview.length > 1;

Because the url only keeps 1 preview and somewhat force our store to only keep 1 preview, the check is obsolete. The new behavior is if 1 preview is present, click go back closes it. We can chat more offline if you want!

PhilippeOberti · 2024-06-28T16:30:56Z

I think a bug might have been introduced with your latest change, the Back button in the top left corner of the preview section always appears, even on the very first preview opened

Yes, this is intended. PR summary now reflect the change:

I guess the cross button in the upper-right corner then is a bit redundant then... but only for the first preview opened. I'm ok with it, but are we sure UIUX and Paul are?

PhilippeOberti

if we're all ok with the back button shown at all times, I think using the browser history for it is the best solution I've seen yet. Awesome job!

christineweng · 2024-06-28T17:09:54Z

I think a bug might have been introduced with your latest change, the Back button in the top left corner of the preview section always appears, even on the very first preview opened

Yes, this is intended. PR summary now reflect the change:

I guess the cross button in the upper-right corner then is a bit redundant then... but only for the first preview opened. I'm ok with it, but are we sure UIUX and Paul are?

@PhilippeOberti this is a compromise we have to have right now.. I'm back from my morning fog 😅, when we use browser history to go back, there is no way of tracking whether we had a preview open or not... unless the urlSync action pass preview as array to redux, but then we need to have all the previews in url, and then the url length may explode...

having the back button always present guarantees user can either go back to previous panel or simply close it. this was it!

ironically the back button is now closer to UIUX's Close, and the close button is the Close all

…ng back (#188703) ## Summary This PR fixes an UI bug. When preview status is tracked in url, opening multiple previews, and clicking `Back` flashes. This is a follow up of the preview logic changes in #186130 To avoid url keeping track of all the previews (which will cause url length explosion), we only keep the last preview in url, and to keep url and redux state in sync, redux also always has 1 preview. Before the fix, we would call `previousPreviewPanelAction`, which empty the preview array and caused the preview panel to disappear. **Before** In a split second, you can see the preview disappears (showing Endpoint security) and then the user preview appears. Redux shows a call of `previousPreviewPanelAction` and it empties the preview array https://github.com/user-attachments/assets/babb12f2-1c1d-422a-87ef-153ed207817b After https://github.com/user-attachments/assets/b2ef891c-181d-4da3-9efb-e4afc7123a99

…ng back (elastic#188703) ## Summary This PR fixes an UI bug. When preview status is tracked in url, opening multiple previews, and clicking `Back` flashes. This is a follow up of the preview logic changes in elastic#186130 To avoid url keeping track of all the previews (which will cause url length explosion), we only keep the last preview in url, and to keep url and redux state in sync, redux also always has 1 preview. Before the fix, we would call `previousPreviewPanelAction`, which empty the preview array and caused the preview panel to disappear. **Before** In a split second, you can see the preview disappears (showing Endpoint security) and then the user preview appears. Redux shows a call of `previousPreviewPanelAction` and it empties the preview array https://github.com/user-attachments/assets/babb12f2-1c1d-422a-87ef-153ed207817b After https://github.com/user-attachments/assets/b2ef891c-181d-4da3-9efb-e4afc7123a99 (cherry picked from commit 001436c)

…hen going back (#188703) (#188766) # Backport This will backport the following commits from `main` to `8.15`: - [[Security Solutioin][Expandable flyout] Fix preview flashing when going back (#188703)](#188703)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: christineweng <18648970+christineweng@users.noreply.github.com>

christineweng added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Threat Hunting:Investigations Security Solution Investigations Team v8.15.0 labels Jun 12, 2024

christineweng requested review from lgestc and PhilippeOberti June 12, 2024 20:09

christineweng self-assigned this Jun 12, 2024

christineweng marked this pull request as ready for review June 12, 2024 20:10

christineweng requested a review from a team as a code owner June 12, 2024 20:10

christineweng commented Jun 12, 2024

View reviewed changes

PhilippeOberti requested changes Jun 17, 2024

View reviewed changes

PhilippeOberti mentioned this pull request Jun 26, 2024

[Security Solution] Enable entity previews in document details flyout #186850

Merged

2 tasks

PhilippeOberti reviewed Jun 26, 2024

View reviewed changes

christineweng force-pushed the bug-multi-preview-url branch from 2b8859a to 53b1b0d Compare June 27, 2024 20:59

christineweng force-pushed the bug-multi-preview-url branch 2 times, most recently from f1d3976 to 569d0e2 Compare June 27, 2024 23:53

fix preview url

8a87162

christineweng force-pushed the bug-multi-preview-url branch from 569d0e2 to 8a87162 Compare June 27, 2024 23:57

PhilippeOberti approved these changes Jun 28, 2024

View reviewed changes

christineweng merged commit c026264 into elastic:main Jun 28, 2024
17 checks passed

PhilippeOberti mentioned this pull request Jul 12, 2024

[Security Solution][Notes] - fix attach to active timeline behavior #188186

Merged

1 task

christineweng mentioned this pull request Jul 18, 2024

[Security Solutioin][Expandable flyout] Fix preview flashing when going back #188703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Details Flyout] Fix multi-preview url sync #186130

[Security Solution][Details Flyout] Fix multi-preview url sync #186130

christineweng commented Jun 12, 2024 •

edited

Loading

elasticmachine commented Jun 12, 2024

christineweng Jun 12, 2024

PhilippeOberti left a comment

PhilippeOberti Jun 17, 2024

PhilippeOberti Jun 17, 2024

christineweng Jun 25, 2024

christineweng Jun 25, 2024

PhilippeOberti left a comment

christineweng commented Jun 27, 2024 •

edited

Loading

christineweng commented Jun 27, 2024

kibana-ci commented Jun 28, 2024

PhilippeOberti commented Jun 28, 2024

PhilippeOberti commented Jun 28, 2024

christineweng commented Jun 28, 2024

christineweng commented Jun 28, 2024

PhilippeOberti commented Jun 28, 2024

PhilippeOberti left a comment

christineweng commented Jun 28, 2024 •

edited

Loading

[Security Solution][Details Flyout] Fix multi-preview url sync #186130

[Security Solution][Details Flyout] Fix multi-preview url sync #186130

Conversation

christineweng commented Jun 12, 2024 • edited Loading

Summary

elasticmachine commented Jun 12, 2024

christineweng Jun 12, 2024

Choose a reason for hiding this comment

PhilippeOberti left a comment

Choose a reason for hiding this comment

PhilippeOberti Jun 17, 2024

Choose a reason for hiding this comment

PhilippeOberti Jun 17, 2024

Choose a reason for hiding this comment

christineweng Jun 25, 2024

Choose a reason for hiding this comment

christineweng Jun 25, 2024

Choose a reason for hiding this comment

PhilippeOberti left a comment

Choose a reason for hiding this comment

christineweng commented Jun 27, 2024 • edited Loading

christineweng commented Jun 27, 2024

kibana-ci commented Jun 28, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

History

PhilippeOberti commented Jun 28, 2024

PhilippeOberti commented Jun 28, 2024

christineweng commented Jun 28, 2024

christineweng commented Jun 28, 2024

PhilippeOberti commented Jun 28, 2024

PhilippeOberti left a comment

Choose a reason for hiding this comment

christineweng commented Jun 28, 2024 • edited Loading

christineweng commented Jun 12, 2024 •

edited

Loading

christineweng commented Jun 27, 2024 •

edited

Loading

christineweng commented Jun 28, 2024 •

edited

Loading