-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[Security Solution][Alert details] - refactor UI on insights #197349
[Security Solution][Alert details] - refactor UI on insights #197349
Conversation
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
9a78583
to
f8d96f9
Compare
Yeah in the ticket I had written a bullet point to discuss these with Paul and Fernanda. On one side I understand how timeline could be nice here, but then it would make the flyout expanded section a bit useless... I'll ask about it on Slack! |
..._solution/public/flyout/document_details/right/components/related_alerts_by_session.test.tsx
Outdated
Show resolved
Hide resolved
...tion/public/flyout/document_details/right/components/related_alerts_by_same_source_event.tsx
Show resolved
Hide resolved
/> | ||
|
||
const onClick = useCallback(() => { | ||
openLeftPanel({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about abstracting all the left panel calls into a hook/helper function? kind of like the cell actions. I see quite a bit of duplication like the params and keys.
Just a thought, could do it in this PR or a separate refactor, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against abstracting, I just wonder to what extend do we push this?
- it seems that the
id
prop is always the same indeed. - for the
path
prop I think it's different enough that it would be passed in to that hook/helper method - for the
params
prop, we mostly useeventId
,scopeId
andindexName
, and while those are mainly retrieved from theuseDocumentDetailsContext
hook, there are instances where they're passed down via props to the component. Would we have then as props to the hook/helper method and default those values internally using theuseDocumentDetailsContext
if they're undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we have then as props to the hook/helper method and default those values internally using the useDocumentDetailsContext if they're undefined?
No, the helper is only for opening left/right in the same context. The simplification would only apply to all the overview headers. Preview or any new flyouts still need a fullopenLeftPanel
call.
I thought about having it part of context, so that more accessible. But obviously the naming convention needs to be distinguishable to the expandable flyout api. Another benefit is we wouldn't need to mock the context values every time.
openLeftPanel({
id: DocumentDetailsLeftPanelKey,
path: {
tab: LeftPanelInsightsTab,
subTab: CORRELATIONS_TAB_ID,
},
params: {
id: eventId,
indexName,
scopeId,
},
});
to something like
openLeft({tab: LeftPanelInsightsTab,subTab: CORRELATIONS_TAB_ID})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. Thanks @PhilippeOberti for responding to my comments.
Only thing I noticed during desk testing: the ... is uncommon
is still present, should they be removed?
/> | ||
|
||
const onClick = useCallback(() => { | ||
openLeftPanel({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we have then as props to the hook/helper method and default those values internally using the useDocumentDetailsContext if they're undefined?
No, the helper is only for opening left/right in the same context. The simplification would only apply to all the overview headers. Preview or any new flyouts still need a fullopenLeftPanel
call.
I thought about having it part of context, so that more accessible. But obviously the naming convention needs to be distinguishable to the expandable flyout api. Another benefit is we wouldn't need to mock the context values every time.
openLeftPanel({
id: DocumentDetailsLeftPanelKey,
path: {
tab: LeftPanelInsightsTab,
subTab: CORRELATIONS_TAB_ID,
},
params: {
id: eventId,
indexName,
scopeId,
},
});
to something like
openLeft({tab: LeftPanelInsightsTab,subTab: CORRELATIONS_TAB_ID})
...tion/public/flyout/document_details/right/components/related_alerts_by_same_source_event.tsx
Show resolved
Hide resolved
f8d96f9
to
2f02ae2
Compare
17b8593
to
75c50ae
Compare
75c50ae
to
2b34b9b
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
History
|
Starting backport for target branches: 8.x |
…#197349) ## Summary This PR refactors the Insights section of the expandable flyout for alerts and events. The changes are applied to the following section: - Threat Intelligence: when the user clicks on the number, we open the left section to the Insights Threat Intelligence tab - Correlations: when the user clicks on the number, we open the left section to the Insights Correlations tab - Prevalence: no user interactions When in preview mode, none of the number are clickable and the buttons are disabled. #### New UI | Normal flyout | Preview flyout | | ------------- | ------------- | | ![Screenshot 2024-10-22 at 6 01 38 PM](https://github.com/user-attachments/assets/de179a2b-c8ab-42f6-b5b7-839dae0139d5) | ![Screenshot 2024-10-22 at 6 01 54 PM](https://github.com/user-attachments/assets/63ed125e-5e3b-4c4c-a10e-7cc01d291660) | #### UX flows to expand the flyout https://github.com/user-attachments/assets/30031a12-c2f3-47e6-a783-5b9482359ee5 elastic/security-team#7033 ### 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 (cherry picked from commit 9b9eb7e)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…197349) (#198417) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution][Alert details] - refactor UI on insights (#197349)](#197349) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Philippe Oberti","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-30T17:33:50Z","message":"[Security Solution][Alert details] - refactor UI on insights (#197349)\n\n## Summary\r\n\r\nThis PR refactors the Insights section of the expandable flyout for\r\nalerts and events.\r\n\r\nThe changes are applied to the following section:\r\n- Threat Intelligence: when the user clicks on the number, we open the\r\nleft section to the Insights Threat Intelligence tab\r\n- Correlations: when the user clicks on the number, we open the left\r\nsection to the Insights Correlations tab\r\n- Prevalence: no user interactions\r\n\r\nWhen in preview mode, none of the number are clickable and the buttons\r\nare disabled.\r\n\r\n#### New UI\r\n\r\n| Normal flyout | Preview flyout |\r\n| ------------- | ------------- |\r\n| ![Screenshot 2024-10-22 at 6 01\r\n38 PM](https://github.com/user-attachments/assets/de179a2b-c8ab-42f6-b5b7-839dae0139d5)\r\n| ![Screenshot 2024-10-22 at 6 01\r\n54 PM](https://github.com/user-attachments/assets/63ed125e-5e3b-4c4c-a10e-7cc01d291660)\r\n|\r\n\r\n#### UX flows to expand the flyout\r\n\r\n\r\nhttps://github.com/user-attachments/assets/30031a12-c2f3-47e6-a783-5b9482359ee5\r\n\r\nhttps://github.com/elastic/security-team/issues/7033\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"9b9eb7e64d59fe41e4513a992ec77eb9fc051e4b","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","v9.0.0","Team:Threat Hunting:Investigations","backport:version","v8.17.0"],"title":"[Security Solution][Alert details] - refactor UI on insights","number":197349,"url":"https://github.com/elastic/kibana/pull/197349","mergeCommit":{"message":"[Security Solution][Alert details] - refactor UI on insights (#197349)\n\n## Summary\r\n\r\nThis PR refactors the Insights section of the expandable flyout for\r\nalerts and events.\r\n\r\nThe changes are applied to the following section:\r\n- Threat Intelligence: when the user clicks on the number, we open the\r\nleft section to the Insights Threat Intelligence tab\r\n- Correlations: when the user clicks on the number, we open the left\r\nsection to the Insights Correlations tab\r\n- Prevalence: no user interactions\r\n\r\nWhen in preview mode, none of the number are clickable and the buttons\r\nare disabled.\r\n\r\n#### New UI\r\n\r\n| Normal flyout | Preview flyout |\r\n| ------------- | ------------- |\r\n| ![Screenshot 2024-10-22 at 6 01\r\n38 PM](https://github.com/user-attachments/assets/de179a2b-c8ab-42f6-b5b7-839dae0139d5)\r\n| ![Screenshot 2024-10-22 at 6 01\r\n54 PM](https://github.com/user-attachments/assets/63ed125e-5e3b-4c4c-a10e-7cc01d291660)\r\n|\r\n\r\n#### UX flows to expand the flyout\r\n\r\n\r\nhttps://github.com/user-attachments/assets/30031a12-c2f3-47e6-a783-5b9482359ee5\r\n\r\nhttps://github.com/elastic/security-team/issues/7033\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"9b9eb7e64d59fe41e4513a992ec77eb9fc051e4b"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197349","number":197349,"mergeCommit":{"message":"[Security Solution][Alert details] - refactor UI on insights (#197349)\n\n## Summary\r\n\r\nThis PR refactors the Insights section of the expandable flyout for\r\nalerts and events.\r\n\r\nThe changes are applied to the following section:\r\n- Threat Intelligence: when the user clicks on the number, we open the\r\nleft section to the Insights Threat Intelligence tab\r\n- Correlations: when the user clicks on the number, we open the left\r\nsection to the Insights Correlations tab\r\n- Prevalence: no user interactions\r\n\r\nWhen in preview mode, none of the number are clickable and the buttons\r\nare disabled.\r\n\r\n#### New UI\r\n\r\n| Normal flyout | Preview flyout |\r\n| ------------- | ------------- |\r\n| ![Screenshot 2024-10-22 at 6 01\r\n38 PM](https://github.com/user-attachments/assets/de179a2b-c8ab-42f6-b5b7-839dae0139d5)\r\n| ![Screenshot 2024-10-22 at 6 01\r\n54 PM](https://github.com/user-attachments/assets/63ed125e-5e3b-4c4c-a10e-7cc01d291660)\r\n|\r\n\r\n#### UX flows to expand the flyout\r\n\r\n\r\nhttps://github.com/user-attachments/assets/30031a12-c2f3-47e6-a783-5b9482359ee5\r\n\r\nhttps://github.com/elastic/security-team/issues/7033\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"9b9eb7e64d59fe41e4513a992ec77eb9fc051e4b"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Philippe Oberti <[email protected]>
Summary
This PR refactors the Insights section of the expandable flyout for alerts and events.
The changes are applied to the following section:
When in preview mode, none of the number are clickable and the buttons are disabled.
New UI
UX flows to expand the flyout
Screen.Recording.2024-10-22.at.6.02.33.PM.mov
https://github.com/elastic/security-team/issues/7033
Checklist