[Security Solution] - Timeline UI refactor #168230

logeekal · 2023-10-06T14:00:25Z

Summary

This PR implements many small refactors in the Timeline UI. I have listed all the changes below which can help you while you are desk testing.

EQL Bar

Before	After

Timeline Title Bar / Bottom Bar

Below screenshots show how timeline bottom bar has changed. Things to note:

Favorite button is now just an icon with the title. User can simply click on it to favorite /un-favorite a timeline

Before	After

Below screenshots show how timeline title bar has changed. Things to note :
- A new timeline action menu has been added to right to timeline title bar.
- All actions such as create a new timeline, a new timeline template. adding timeline to case, etc can be performed from here.

Before	After

On the left side of the Timeline Header below are the changes.
- Timeline Title is not longer a button/link, so timeline cannot be closed by clicking on that.
- ⊕ action menu is not longer available and corresponding actions are available in above screenshots.

Before	After

Timeline Header Panel

Below timeline header panel has been completely removed.

Changes on how Data provider works

Data provider is by-default hidden in normal timeline but visible in template timeline.
Data provider can be toggled by the user on-demand.
Data Provider will automatically become visible if user wants to put a data grid column value in data provider and stars dragging it. Below videos shows how that interaction works.

Data.Provider.Interaction.mov

KPI

This PR also changes how KPIs are visible in empty and populated state.

Before	After

KPI bar has been completely removed till this issue resolves: #171569

Query Bar

In contrast to current layout of the query bar, DataView picker, Query bar and Date Picker has been brought in the same line. This was done in an effort to make it uniform in looks w.r.t the global query bar.

Before

After

All the highlighted components are in the same line now + A button to toggle Data Provider ( as explained in Data Porvider/QueryBuilder Section) has also been added.

Spacing Uniformity

In the existing version of timeline, spacing is different at many places. This PR aims to bring some uniformity to those spacing decisions ( primarily in EQL and Query Tab). The changes are very minor visually, please feel free to find and report any discrepancies.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

…fix'

…-fix'

michaelolo24 · 2023-11-01T16:42:27Z

Nice work with these changes @logeekal ! Quick question, could we also re-order the items on the correlation tab to align with the query tab as well?

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

logeekal · 2023-11-03T09:01:10Z

Could we also re-order the items on the correlation tab to align with the query tab as well?

@michaelolo24 , I thought about it.. but the problem with that is query bar. it has different height so I could not think of a good way, they will fit together. so i left it alone. I will check it out again.

e40pud

Detection engine changes LGTM

...ck/plugins/security_solution/public/timelines/components/flyout/action_menu/new_timeline.tsx

x-pack/plugins/security_solution/public/timelines/components/flyout/bottom_bar/index.tsx

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

PhilippeOberti · 2023-11-22T17:09:14Z

@PhilippeOberti , I updated the changes suggested by you here.

It looks awesome thank you!

there is a rounded border to the KPIs section that looks weird with the vertical line to its left and the horizontal line to its bottom

Sorry, I do not understand which vertical and horizontal lines you are referring to. The curve is to match the design on the above query bar which also has the curved at the corners. Honestly, I wanted to curve the filters as well but was not sure so left it alone.

Nonetheless, as per @paulewing 's suggestion, I have, for the time being removed the KPI section till #171569 is resolved.

Yeah I just pulled down the branch and was wondering where it went. It makes sense and I remember Paul saying he wasn't sure about that section!

PhilippeOberti

Left a few comments.

Regarding the React.memo, I did point out to some of the places where we are memoizing component twice, but maybe not all of them...
Same thing for the multiple exports. I noticed a few but might have missed some...

I would consider having always a single export and have the memoization done there as well. Instead of having this

const MyBestComponent = {...}

export const MyBest = React.memo(MyBestComponent)

I think we should always to this

export const MyBestComponent = React.memo({...})

unless we have a good reason not to do this. I find it annoying, that when doing a repo-wide search or when navigating from one component to another, you only find or land on that export line at the bottom of the file...

Another thing to be careful about, especially with timeline, is modifying components that are used outside of timeline, and there are lots of them. I've been wanting to extract these for a while but haven't had the time yet. I've been trying to test things outside of timeline and everything looks good so far, but we need to keep an eye on these

PhilippeOberti · 2023-11-22T17:29:15Z

...ns/security_solution/public/timelines/components/flyout/action_menu/save_timeline_button.tsx

-SaveTimelineButton.displayName = 'SaveTimelineButton';
+SaveTimelineButtonComponent.displayName = 'SaveTimelineButton';
+
+export const SaveTimelineButton = React.memo(SaveTimelineButtonComponent);


aren't we memoizing this component twice?

fixed in af2d737

PhilippeOberti · 2023-11-22T17:29:43Z

...k/plugins/security_solution/public/timelines/components/flyout/action_menu/open_timeline.tsx

+
+OpenTimelineActionComponent.displayName = 'OpenTimelineModalButton';
+
+export const OpenTimelineAction = React.memo(OpenTimelineActionComponent);


here as well, aren't we memoizing this twice?

fixed in af2d737

PhilippeOberti · 2023-11-22T17:40:46Z

...ins/security_solution/public/timelines/components/flyout/action_menu/save_timeline_modal.tsx

-SaveTimelineModal.displayName = 'SaveTimelineModal';
+SaveTimelineModalComponent.displayName = 'SaveTimelineModal';
+
+export const SaveTimelineModal = React.memo(SaveTimelineModalComponent);


here as well

fixed in af2d737

PhilippeOberti · 2023-11-22T17:43:44Z

...plugins/security_solution/public/timelines/components/flyout/header/timeline_status_info.tsx

I feel like this file would be a very good candidate for some unit tests

fixed in af2d737

PhilippeOberti · 2023-11-22T17:57:20Z

...ins/security_solution/public/timelines/components/side_panel/event_details/flyout/footer.tsx

+const StyledFooter = styled(EuiFlyoutFooter)`
+  padding-inline: ${({ theme }) => theme.eui.euiSizeM};
+  padding-block: ${({ theme }) => theme.eui.euiSizeM};
+  background-color: transparent;
+`;


this is risky because that flyout footer is used in many places outside of timeline (for example it's used in the expandable flyout.

I don't think it's a really good pattern to have the lower level reusable component have css properties like padding and background colors. I think we should leave that up to the parent components using them, unless we want to force something.

I feel like instead of modifying this component, we should wrap it wherever it's used. We actually did this for the expandable flyout (see here) where we wrap it with a EuiPanel with the correct backgroundColor.

Also while I understand the padding change, I'm curious about why removing the light blue background?

It's probably not a big deal because that whole side panel thing will be good soonish and replaced by the expandable flyout...

We should also consider moving these reusable components to /public/common/components, this would let us know the changes may affect different places. The expandable flyout should not be using components/hooks from the Timeline directory.

yes it shouldn't, and yes we should move these components out, but this is a much bigger effort and I believe should be done in a separate PR. It will be a nice cleanup though!

EuiFlyoutFooter has certain built-in styles that only work when it is nested inside EuiFlyout. Since timeline is not nested in a EuiFlyout parent, those styles do not apply Because of this, timeline side panel footer looks like below ( which is pretty bad and the reason I made it transparent ):

instead of looking like this:

Infact, the whole, Timeline body ( table, and side panel ) styling is currently very messy becuase of which this patch work need to be applied.

This particular components is not reusable and is not use used anywere else so I think this part of styling is okay. And any other kind of refactoring should be taken separate PR.

This particular components is not reusable and is not use used anywere else so I think this part of styling is okay. And any other kind of refactoring should be taken separate PR.

while this specific StyledFooter component isn't used anywhere, like I mentioned in you are using it within FlyoutFooterComponent which IS used in other places (expandable flyout), that's why I wanted to raise a point of making sure these other places were checked as part of the PR review. That's it 😄

Hmm.. okay thanks for pointing that out. Let me double checks its impact on expandable flyout or anywhere it is used.

@PhilippeOberti , Although, it does not impact Expandable flyout and expandable flyout applies it own style on this footer because current styles are just wrong.

But I understand your point and I have lifted the style customization one component above which specific to timeline.

Additionally, it is not used anywhere else.

Added here : 05d764c

PhilippeOberti · 2023-11-22T18:06:22Z

x-pack/plugins/security_solution/public/timelines/components/timeline/kpi/kpi_container.tsx

I would consider removing these files entirely. I understand that we will add the kpis back at some point, but it might take weeks or months before that happens... and when that happens the UI or content might look very different...

During that time we'd have to potentially maintain these files. Also someone external would be confused by seeing these files in the repository not understanding why they're not used, and coming back to the PR he/she would have to go through 70+ comments to find the one that mentions we're hiding KPIs for now...

I would remove all these newly created files as well as the few places where their usage is commented out. You can keep them on a local branch until the discussion of bringing them back happens?

PhilippeOberti · 2023-11-22T18:09:17Z

x-pack/plugins/security_solution/public/timelines/components/timeline/query_bar/index.tsx

+const SearchBarContainer = styled.div`
+  /*
+  *
+  * hide search bar default filters as they are distrubing the layout  as shown below


distrubing => disturbing

fixed here: 05b43e2

PhilippeOberti · 2023-11-22T18:09:52Z

x-pack/plugins/security_solution/public/timelines/components/timeline/query_bar/index.tsx

+  *                    |      Filters                 |
+  *                    --------------------------------
+  *
+  * The tree under this component makes sure that default filters are not rendered and we can saperately display


saperately => separately

This spell checker extension for VSCode works awesome for me. Other IDEs should have similar things.

yes, I have that as well but some gets left behind since pre-commit check doesn't include that check. Will try to see if include that locally.

fixed here: 05b43e2

PhilippeOberti · 2023-11-22T18:14:08Z

...k/plugins/security_solution/public/timelines/components/timeline/query_tab_content/index.tsx

+              {/* TODO: This is a temporary solution to hide the KPIs until lens components play nicely with timelines */}
+              {/* https://github.com/elastic/kibana/issues/17156 */}
+              {/* <EuiFlexItem grow={false}> */}
+              {/*   <TimelineKpi timelineId={timelineId} /> */}
+              {/* </EuiFlexItem> */}


I would consider deleting the commented code. I mentioned it somewhere else but there are a lot of new files introduced by the PR that I don't think should be added either. It introduces confusion (especially for the new files) and they might be completely changed in the future...

PhilippeOberti · 2023-11-22T18:17:09Z

x-pack/plugins/security_solution/public/timelines/store/timeline/reducer.ts

@@ -530,6 +531,18 @@ export const timelineReducer = reducerWithInitialState(initialTimelineState)
      },
    },
  }))
+  .case(setDataProviderVisibility, (state, { id, isDataProviderVisible }) => {


I haven't looked at the way we test these reducers, but shouldn't we add some unit tests when introducing a new case?

Currently we do not have test for reducers, I think it is because they are very atomic piece of functionality where we are simple using external library to modify the state.

However, there are some complex actions which make use of some helpers when reducing the state. There was a test file for those called reducer.test.tsx. I have renamed that to helpers.test.tsx in af2d737

semd · 2023-11-23T09:28:11Z

@PhilippeOberti

Instead of having this
const MyBestComponent = {...}
export const MyBest = React.memo(MyBestComponent)

I think we should always to this
export const MyBestComponent = React.memo({...})

I agree, I always try to export the memo return directly for the same reason. However, this syntax causes the linter to complain about a missing displayName. So I am using a named function inside the memo to prevent that linter error from showing up:

export const HelloWorld = React.memo<Props>(function HelloWorld(props) {
  return <>Hello World</>;
});

BTW, I would also forbid the use of the word Component in the exported component names, if we do that we should do it to all components, otherwise we should never do it. And we use typescript to know what things are, so no need to always append Component to the component name.

x-pack/plugins/security_solution/public/timelines/components/timeline/query_bar/index.tsx

PhilippeOberti · 2023-11-23T15:45:28Z

BTW, I would also forbid the use of the word Component in the exported component names, if we do that we should do it to all components, otherwise we should never do it. And we use typescript to know what things are, so no need to always append Component to the component name.

yes absolutely agree, it was a mistake on my end to add the word Component, I actually hate it as well and never do it... 😆

logeekal · 2023-11-23T16:01:54Z

yes absolutely agree, it was a mistake on my end to add the word Component, I actually hate it as well and never do it... 😆

I guess, we all agree on this.

semd

LGTM! Great work Jatin. Thanks for the effort 🚀

logeekal · 2023-11-24T15:31:05Z

x-pack/test/security_solution_cypress/cypress/tasks/api_calls/timelines.ts

+    headers: {
+      'kbn-xsrf': 'cypress-creds',
+      'x-elastic-internal-origin': 'security-solution',
+      'elastic-api-version': '2023-10-31',
+    },


@MadameSheema , Please do let me know if this is not correct.

kibana-ci · 2023-11-26T20:26:08Z

💛 Build succeeded, but was flaky

Failed CI Steps

Defend Workflows Cypress Tests on Serverless #3

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4727	4726	-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
`securitySolution`	106	107	+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`	12.8MB	12.8MB	-19.6KB
`unifiedSearch`	213.8KB	213.9KB	+118.0B
total			-19.5KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`securitySolution`	65.3KB	65.4KB	+95.0B

Unknown metric groups

API count

id	before	after	diff
`securitySolution`	175	176	+1

ESLint disabled line counts

id	before	after	diff
`securitySolution`	468	464	-4

Total ESLint disabled count

id	before	after	diff
`securitySolution`	538	534	-4

History

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

cc @logeekal

PhilippeOberti

awesome changes, congrats and thanks, that was a huge amount of work!

feat/intial_timeline_ui

54bf025

logeekal added Team:Threat Hunting:Investigations Security Solution Investigations Team ci:cloud-deploy Create or update a Cloud deployment ci:cloud-persist-deployment Persist cloud deployment indefinitely labels Oct 6, 2023

logeekal added 5 commits October 12, 2023 16:24

more experimental UI

69d6538

incremental save

1f4dd53

incremental save

b3a13bd

feat: added timeline action menu + fixed filter badges

b5c17d7

timeline ui all changes

ef13b22

logeekal force-pushed the expr/new_timeline_ui branch from 90463b0 to ef13b22 Compare October 30, 2023 09:56

kibanamachine and others added 4 commits October 30, 2023 10:03

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

133756f

…fix'

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

70002c6

…-fix'

fix: add missing translations

ba4bcc0

Merged main -> current branch

74b3cb8

logeekal marked this pull request as ready for review October 30, 2023 14:58

logeekal requested review from a team as code owners October 30, 2023 14:58

logeekal changed the title ~~[Security Solution] [Experimental] - Timeline UI refactor~~ [Security Solution] - Timeline UI refactor Oct 30, 2023

fix: types + some tests

eb76e7f

logeekal added the release_note:enhancement label Oct 31, 2023

fix: more tests

fee5b8c

michaelolo24 reviewed Nov 1, 2023

View reviewed changes

x-pack/plugins/security_solution/public/common/components/query_bar/index.tsx Outdated Show resolved Hide resolved

e40pud approved these changes Nov 3, 2023

View reviewed changes

michaelolo24 reviewed Nov 3, 2023

View reviewed changes

...ck/plugins/security_solution/public/timelines/components/flyout/action_menu/new_timeline.tsx Outdated Show resolved Hide resolved

michaelolo24 reviewed Nov 3, 2023

View reviewed changes

x-pack/plugins/security_solution/public/timelines/components/flyout/bottom_bar/index.tsx Show resolved Hide resolved

michaelolo24 reviewed Nov 3, 2023

View reviewed changes

x-pack/plugins/security_solution/public/timelines/components/flyout/header/index.test.tsx Outdated Show resolved Hide resolved

logeekal requested a review from semd November 22, 2023 14:50

fix:filter-search-popover-width

bfc1246

PhilippeOberti reviewed Nov 22, 2023

View reviewed changes

semd reviewed Nov 23, 2023

View reviewed changes

x-pack/plugins/security_solution/public/timelines/components/timeline/query_bar/index.tsx Outdated Show resolved Hide resolved

logeekal added 3 commits November 23, 2023 17:47

Merged main -> current branch

53c6c39

fix: review feedback

af2d737

Merge branch 'main' into expr/new_timeline_ui

7451a7d

elastic deleted a comment from kibana-ci Nov 24, 2023

logeekal added 3 commits November 24, 2023 13:23

fix: revert some changes

ac257e3

fix: remove data-test-subj from styles

ef489cf

fix: spellings

05b43e2

semd approved these changes Nov 24, 2023

View reviewed changes

logeekal added 2 commits November 24, 2023 15:14

fix: flyout footer

05d764c

fix: cypress test

2ed673e

logeekal commented Nov 24, 2023

View reviewed changes

logeekal added 3 commits November 25, 2023 23:44

Merge branch 'main' into expr/new_timeline_ui

304b99a

fix: misc

5273e4c

Merge branch 'main' into expr/new_timeline_ui

bb377ed

PhilippeOberti approved these changes Nov 27, 2023

View reviewed changes

logeekal merged commit 72d2457 into elastic:main Nov 27, 2023

kibanamachine added v8.12.0 backport:skip This commit does not require backporting labels Nov 27, 2023

logeekal removed the ci:cloud-persist-deployment Persist cloud deployment indefinitely label Nov 27, 2023

nastasha-solomon mentioned this pull request Dec 2, 2023

Timeline UI changes tour copy review elastic/security-docs#4369

Closed


		OpenTimelineActionComponent.displayName = 'OpenTimelineModalButton';

		export const OpenTimelineAction = React.memo(OpenTimelineActionComponent);

[Security Solution] - Timeline UI refactor #168230

[Security Solution] - Timeline UI refactor #168230

Conversation

logeekal commented Oct 6, 2023 • edited Loading

Summary

EQL Bar

Timeline Title Bar / Bottom Bar

Timeline Header Panel

Changes on how Data provider works

KPI

Query Bar

Before

After

Spacing Uniformity

Checklist

michaelolo24 commented Nov 1, 2023

logeekal commented Nov 3, 2023

e40pud left a comment

Choose a reason for hiding this comment

PhilippeOberti commented Nov 22, 2023

PhilippeOberti 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

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

semd Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logeekal Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logeekal Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

logeekal Nov 24, 2023 • 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

logeekal Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

semd commented Nov 23, 2023

PhilippeOberti commented Nov 23, 2023

logeekal commented Nov 23, 2023

semd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Nov 26, 2023 • edited Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

PhilippeOberti left a comment

Choose a reason for hiding this comment

logeekal commented Oct 6, 2023 •

edited

Loading

PhilippeOberti left a comment •

edited

Loading

semd Nov 23, 2023 •

edited

Loading

logeekal Nov 23, 2023 •

edited

Loading

logeekal Nov 23, 2023 •

edited

Loading

logeekal Nov 24, 2023 •

edited

Loading

logeekal Nov 24, 2023 •

edited

Loading

kibana-ci commented Nov 26, 2023 •

edited

Loading