[Security Solution][Investigations] Alert flyout UX updates (pt. 1) #120347

janmonschke · 2021-12-03T14:13:55Z

Summary

The changes in this PR represent the first set of tasks for https://github.com/elastic/security-team/issues/1764. It focuses on adding "overview cards" to the alert flyout. The content for those cards is moved from the document summary to just below the tabbed navigation:

Screen.Recording.2021-12-03.at.14.59.02.mov

(Figma link)

Most changes are of visual nature and required adding new components. You will find comments next to the changes where I refactored logic or other code.

Todo

Talk to @monina-n about changing the title template to ${ruleName} alert
Some strings where added and probably removed, I'll check how to make sure to follow the i18n guidelines

elasticmachine · 2021-12-03T14:13:58Z

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

elasticmachine · 2021-12-03T14:13:58Z

Pinging @elastic/security-solution (Team: SecuritySolution)

janmonschke · 2021-12-03T14:15:27Z

x-pack/plugins/security_solution/public/common/components/event_details/alert_summary_view.tsx

  const summaryRows = useMemo(
    () => getSummaryRows({ browserFields, data, eventId, isDraggable, timelineId }),
    [browserFields, data, eventId, isDraggable, timelineId]
  );

  return (
-    <>
-      <EuiSpacer size="s" />


💅 I moved this spacer to the parent so it has full control over the layout and we can get rid of the ReactFragment

nice refactor @janmonschke! 🚀

janmonschke · 2021-12-03T14:16:35Z

x-pack/plugins/security_solution/public/common/components/event_details/alert_summary_view.tsx

@@ -57,18 +57,21 @@ const AlertSummaryViewComponent: React.FC<{
  eventId: string;
  isDraggable?: boolean;
  timelineId: string;
-  title?: string;
-}> = ({ browserFields, data, eventId, isDraggable, timelineId, title }) => {
+  title: string;


💅 title was defined in all cases apart from tests. Made it not optional and updated the test data.

janmonschke · 2021-12-03T14:17:24Z

.../plugins/security_solution/public/common/components/event_details/get_alert_summary_rows.tsx

@@ -151,50 +129,34 @@ export const getSummaryRows = ({
  const tableFields = getEventFieldsToDisplay({ eventCategory, eventCode });

  return data != null
-    ? tableFields.reduce<SummaryRow[]>((acc, item) => {
-        const initialDescription = {


I moved all this logic to getEnrichedFieldInfo to make it reusable in other places.

renaming item to field in this reduce is a subtle, yet appreciated improvement to the readability when it's used below 🙏

janmonschke · 2021-12-03T14:18:49Z

x-pack/plugins/security_solution/public/common/components/event_details/reason.tsx

  if (!eventId) {
    return <EuiTextColor color="subdued">{EVENT_DETAILS_PLACEHOLDER}</EuiTextColor>;
  }

-  return reason ? (


The link to the rule now lives in one of the cards in the overview, so we could remove it from here.

janmonschke · 2021-12-03T14:23:12Z

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

@@ -227,14 +232,18 @@ export const HoverActions: React.FC<Props> = React.memo(
      values,
    });

+    const Container = applyWidthAndPadding


We wanted to use these actions in the overview cards where we don't want them to have a fixed width. Therefore I created two versions of it. One with and one without the extra width declarations. Alternatively I could have removed the min-width and padding altogether from this component to make it easier to integrate into other layouts. Essentially moving the responsibility to the parent component. If you feel like that is the way I should have done it, I'm happy to do it that way also.

janmonschke · 2021-12-03T14:23:41Z

...lugins/security_solution/public/timelines/components/timeline/body/renderers/rule_status.tsx

 }) => {
  const color = useMemo(() => getOr('default', `${value}`, mapping), [value]);
+  const badge = (


💅 💅 💅

probably want to move this definition outside of this component

You mean outside as a standalone component instead of inlining the component?

janmonschke · 2021-12-03T14:24:46Z

@elasticmachine merge upstream

kibanamachine · 2021-12-03T14:24:48Z

merge conflict between base and head

.../security_solution/public/common/components/event_details/overview/status_popover_button.tsx

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

.../plugins/security_solution/public/common/components/event_details/get_alert_summary_rows.tsx

x-pack/plugins/security_solution/public/common/components/event_details/helpers.tsx

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

janmonschke · 2021-12-06T09:21:53Z

@elasticmachine merge upstream

This method can be used in different places to enrich the field data.

Ideally, ActionCell and HoverActions would not have padding and width declaration. This could be part of a future refactor. For now, a version with padding and size information is all that is needed.

michaelolo24 · 2021-12-08T16:20:40Z

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

+import { useThrottledResizeObserver } from '../../utils';
+
+export const NotGrowingFlexGroup = euiStyled(EuiFlexGroup)`
+  flex-grow: 0;


I think there is a grow prop on the flex group or maybe the flex item.

I tried to add grow={false} to the EuiFlexItems that are rendered by this but it did not do the trick as the parent (the EuiFlexGroup) was still expanding to full height. Probably because it's nested in another FlexGroup from outside?

michaelolo24 · 2021-12-08T16:36:13Z

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

+      );
+    }, [browserFields, contextId, data, eventId, timelineId]);
+
+    const signalCard = hasData(statusData) && (


What are your thoughts on separate components that live outside of the overview component? Since all of these are essentially presentational, I don't expect any updates in them that will cause the whole component to re-render with any significant frequency, but I think it pre-emptively keeps this overview component from becoming overly bloated as time progresses if/when other functionality is added. You could have separate card components, or just one card component that takes the data, dataType(ruleName, signal, etc..), and maybe a grow property if that works to achieve the intended view when resizing

Agreed, I think the next time this file is touched / appended-to, we could do this. Or do you see this as a blocker for merging this PR?

Not a blocker. Just wanted to point it out 😄 Thanks!

michaelolo24 · 2021-12-08T16:39:30Z

.../plugins/security_solution/public/common/components/event_details/overview/overview_card.tsx

+`;
+
+const OverviewPanel = euiStyled(EuiPanel)`
+  &&& {


TIL. Had never seen &&& before.

Also, not to come in last minute here. Just wanted to make sure that we didn't want to use the EUICard for this: https://elastic.github.io/eui/#/display/card#custom-children

Haha, yeah, &&& is fun and it looks a lot less like a hack than .myClass.myClass.myClass even though it's exactly the same :D

Regarding using EUICard: I looked into it in the beginning but I would have ha to override a lot of styles and then decided to go with the custom components.

Sounds good. Thanks!

andrew-goldstein

Thanks for this enhancement @janmonschke !
Desk tested above & below the layout breakpoint in:

Chrome 96.0.4664.93
Firefox 95.0
Safari 15.1

LGTM 🚀

janmonschke · 2021-12-09T13:57:06Z

Found another tiny rendering edge-case. If not all cards are available, it would render unwanted combination of rows:

The fix was to use a smarter way to chunk the rows up into rows of 2: e6cf74c

janmonschke · 2021-12-09T13:57:31Z

@elasticmachine merge upstream

michaelolo24

Thanks for the great work LGTM!

kibana-ci · 2021-12-09T20:23:18Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Security Solution Tests / Row renderers Suricata Signature tooltips do not overlap

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2806	2809	+3

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.5MB	4.6MB	+5.7KB

History

💔 Build #12458 failed 21819ea
💔 Build #12432 failed fc686b7
💔 Build #12155 failed a487472
💔 Build #11721 failed 30d16a7
💔 Build #11212 failed 250152b

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

cc @janmonschke

kibanamachine · 2021-12-10T10:30:25Z

The following labels were identified as gaps in your version labels and will be added automatically:

v8.1.0

If any of these should not be on your pull request, please manually remove them.

kibanamachine · 2021-12-10T10:32:48Z

💔 Backport failed

Status	Branch	Result
❌	8.0	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 120347

kibanamachine · 2021-12-13T10:35:01Z

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

…lastic#120347) * feat: Move timestamp from summary to below the title * refactor: creat reusable getEnrichedFieldInfo This method can be used in different places to enrich the field data. * feat: make unpadded/unsized version of ActionCell available Ideally, ActionCell and HoverActions would not have padding and width declaration. This could be part of a future refactor. For now, a version with padding and size information is all that is needed. * feat: add OverviewCards w/ severity, risk score and rule name * feat: add status to overview cards * refactor: use FormattedFieldValue instead of RuleStatus directly * fix: limit height of the overview cards * fix: clamp content to 2 lines * chore: add displayName * feat: Add interactive popover to status badge * chore: remove signal status from document summary * feat: Remove rule link and headline from reason component * feat: Add table-tab pivot link * feat: close alert flyout after status change * test: fix snapshots * chore: remove unused imports * chore: use correct padding in context menu * chore: split over cards into multiple files * chore: use shared severity badge * chore: revert back to plain risk score text * chore: rename and move overview * fix: fix alignment between actions and content * fix: fix types in test * chore: remove unused import * chore: useMemo & useCallback * chore: import type * feat: add iconType, iconSide and onClickArialabel to rule status * feat: add hover actions to the alert status overview card * fix: use correct data * fix: action cell did not look good on small screens Now the action cell slides in similar to how the action buttons slide in in a data grid. * fix: use different card layout based on container width * fix: use new Severity type * fix: align children centered * test: add popover button tests * test: add overview card test * test: test overview cards * fix: prevent rendering of two cards in two ingle rows * fix: change i18n key to prevent a duplicate key * chore: remove unused translations * nit: use less vertical screen estate Co-authored-by: Kibana Machine <[email protected]>

janmonschke requested a review from a team as a code owner December 3, 2021 14:13

janmonschke commented Dec 3, 2021

View reviewed changes

janmonschke added the release_note:enhancement label Dec 3, 2021

janmonschke force-pushed the security/alert-flyout-ux-updates branch from bde5ec3 to b873bc2 Compare December 3, 2021 15:13