Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Security Solution][Timeline] extract and cleanup timeline bottom bar component #173886

Merged
merged 4 commits into from
Jan 11, 2024
Merged

[Security Solution][Timeline] extract and cleanup timeline bottom bar component #173886

merged 4 commits into from
Jan 11, 2024

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented Dec 21, 2023 •
edited
Loading

Notes

This is a follow up PR to #173880 which should be merged first. The changes can be reviewed individually though, by ignoring the first 2 commits and focusing only on the third one.

This PR is part of a long-running effort to clean up the timeline code. I originally had opened #172408 but the file count kept growing and so did the complexity for the reviewers. Of that original PR I will create probably 5-6 smaller ones.

Summary

This very PR focuses on separating the FlyoutBottomBar (the timeline bottom bar) from the StatefulTimelineComponent (loaded inside the modal). Those 2 components were using the same FlyoutHeaderPanelComponent but the content of that component wasn't actually common and lots of if/else condition existed to support both usages.

First extract the FlyoutBottomBar component:

  • extract the component out of the flyout folder as it is actually loaded within the timeline wrapper component, not the flyout
  • rename the folder and file to bottom_bar and TimelineBottomBar respectively to better represent what the component actually does
  • use the individual components instead of using the FlyoutHeaderPanelComponent directly, which doesn't really make much sense
  • add documentation and improve unit tests

The PR also removes the ActiveTimelines component which was used in the FlyoutHeaderPanelComponent but isn't needed anymore, as the modal header can render the title directly.
Also a new useTimelineTitle is introduced to always return the correct timeline title (handles saved, draft and template states)

Absolutely no visual or behavior changes introduced here!

Checklist

The following PR will focus on cleanup up the flyout folder.

@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team labels Dec 21, 2023
@PhilippeOberti PhilippeOberti requested review from a team as code owners December 21, 2023 23:59
@PhilippeOberti PhilippeOberti changed the title Timeline cleanup part 3 [Security Solution][Timeline] extract and cleanup timeline bottom bar component Dec 26, 2023
@PhilippeOberti PhilippeOberti force-pushed the timeline-cleanup-part-3 branch 2 times, most recently from acca57e to aa2075d Compare January 9, 2024 22:19
@PhilippeOberti
Copy link
Contributor Author

Files by Code Owner

elastic/kibana-localization

  • x-pack/plugins/translations/translations/fr-FR.json
  • x-pack/plugins/translations/translations/ja-JP.json
  • x-pack/plugins/translations/translations/zh-CN.json

elastic/protections-experience

  • x-pack/plugins/threat_intelligence/cypress/screens/timeline.ts

elastic/security-defend-workflows

  • x-pack/plugins/osquery/cypress/e2e/all/alerts_automated_action_results.cy.ts
  • x-pack/plugins/osquery/cypress/e2e/all/alerts_linked_apps.cy.ts
  • x-pack/plugins/osquery/cypress/e2e/all/timelines.cy.ts
  • x-pack/plugins/security_solution/public/management/pages/endpoint_hosts/view/index.test.tsx
  • x-pack/plugins/security_solution/public/management/pages/policy/view/policy_details.test.tsx

elastic/security-engineering-productivity

  • x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/open_timeline.cy.ts
  • x-pack/test/security_solution_cypress/cypress/screens/security_main.ts
  • x-pack/test/security_solution_cypress/cypress/screens/timeline.ts
  • x-pack/test/security_solution_cypress/cypress/tasks/timeline.ts

elastic/security-solution

  • x-pack/plugins/security_solution/public/management/pages/endpoint_hosts/view/index.test.tsx
  • x-pack/plugins/security_solution/public/management/pages/policy/view/policy_details.test.tsx
  • x-pack/plugins/security_solution/public/timelines/components/bottom_bar/index.test.tsx
  • x-pack/plugins/security_solution/public/timelines/components/bottom_bar/index.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/bottom_bar/index.test.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/bottom_bar/index.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/bottom_bar/translations.ts
  • x-pack/plugins/security_solution/public/timelines/components/flyout/header/active_timelines.test.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/header/active_timelines.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/header/index.tsx
  • x-pack/plugins/security_solution/public/timelines/components/timeline/helpers.tsx
  • x-pack/plugins/security_solution/public/timelines/hooks/use_timeline_title.test.tsx
  • x-pack/plugins/security_solution/public/timelines/hooks/use_timeline_title.tsx
  • x-pack/plugins/security_solution/public/timelines/wrapper/index.test.tsx
  • x-pack/plugins/security_solution/public/timelines/wrapper/index.tsx

elastic/security-threat-hunting-investigations

  • x-pack/plugins/security_solution/public/timelines/components/bottom_bar/index.test.tsx
  • x-pack/plugins/security_solution/public/timelines/components/bottom_bar/index.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/bottom_bar/index.test.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/bottom_bar/index.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/bottom_bar/translations.ts
  • x-pack/plugins/security_solution/public/timelines/components/flyout/header/active_timelines.test.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/header/active_timelines.tsx
  • x-pack/plugins/security_solution/public/timelines/components/flyout/header/index.tsx
  • x-pack/plugins/security_solution/public/timelines/components/timeline/helpers.tsx
  • x-pack/plugins/security_solution/public/timelines/hooks/use_timeline_title.test.tsx
  • x-pack/plugins/security_solution/public/timelines/hooks/use_timeline_title.tsx
  • x-pack/plugins/security_solution/public/timelines/wrapper/index.test.tsx
  • x-pack/plugins/security_solution/public/timelines/wrapper/index.tsx
  • x-pack/test/security_solution_cypress/cypress/e2e/investigations/timelines/open_timeline.cy.ts

paul-tavares
paul-tavares approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - From Defend Workloads team 👍

@janmonschke
Copy link
Contributor

@PhilippeOberti What's the advantage of using useTimelineTitle over using a dedicated component (<TimelineTitle />)? Since the return value of useTimelineTitle is only used for rendering, it feels like it should be a component. A component can also be memoized easier and is easier to test. Wdyt?

tomsonpl
tomsonpl approved these changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defend workflows lgtm 👍

lgestc
lgestc approved these changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@lgestc lgestc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit, feel free to ignore it but ternaries are something I dont like :D

x-pack/plugins/security_solution/public/timelines/hooks/use_timeline_title.tsx Outdated Show resolved Hide resolved
@PhilippeOberti
Copy link
Contributor Author

@PhilippeOberti What's the advantage of using useTimelineTitle over using a dedicated component (<TimelineTitle />)? Since the return value of useTimelineTitle is only used for rendering, it feels like it should be a component. A component can also be memoized easier and is easier to test. Wdyt?

@janmonschke The reason I chose a hook over a component here was because I specifically did not want to have any display/rendering in it. That hook is used in 2 places: the timeline bottom bar and the header of the timeline modal. Both display the same value, but the way they are displayed is different:

  • normal size text that has an underline when mousing over with ability to click on it for the bottom bar
  • bigger size text with no mouse over action or click event for the modal

That might not be a good enough reason to have it a hook vs a component though, not sure honestly... I just feel like I would hate having a component with an if/else condition to handle those 2 situations, and I also dislike the option of passing the some css or classname as props to drive the display.
Any ideas how to do it better?

@PhilippeOberti
Copy link
Contributor Author

@PhilippeOberti What's the advantage of using useTimelineTitle over using a dedicated component (<TimelineTitle />)? Since the return value of useTimelineTitle is only used for rendering, it feels like it should be a component. A component can also be memoized easier and is easier to test. Wdyt?

@janmonschke The reason I chose a hook over a component here was because I specifically did not want to have any display/rendering in it. That hook is used in 2 places: the timeline bottom bar and the header of the timeline modal. Both display the same value, but the way they are displayed is different:

  • normal size text that has an underline when mousing over with ability to click on it for the bottom bar
  • bigger size text with no mouse over action or click event for the modal

That might not be a good enough reason to have it a hook vs a component though, not sure honestly... I just feel like I would hate having a component with an if/else condition to handle those 2 situations, and I also dislike the option of passing the some css or classname as props to drive the display. Any ideas how to do it better?

After talking to the team during standup, I'm going to create a selector for this instead. No need of a component or a hook! :)

@PhilippeOberti
Copy link
Contributor Author

PhilippeOberti commented Jan 10, 2024 •
edited
Loading

After talking to the team during standup, I'm going to create a selector for this instead. No need of a component or a hook! :)

implemented here

@PhilippeOberti PhilippeOberti force-pushed the timeline-cleanup-part-3 branch 2 times, most recently from 9f4c86e to 5e7c8ab Compare January 10, 2024 22:35
@PhilippeOberti PhilippeOberti requested a review from a team as a code owner January 10, 2024 22:35
nkhristinin
nkhristinin approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@nkhristinin nkhristinin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cypress tests for sourcerer LGTM!

janmonschke
janmonschke approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@janmonschke janmonschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4868 4867 -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 11.4MB 11.4MB -11.0KB

History

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

@PhilippeOberti PhilippeOberti merged commit 2b45409 into elastic:main Jan 11, 2024
37 checks passed
@PhilippeOberti PhilippeOberti deleted the timeline-cleanup-part-3 branch January 11, 2024 16:02
@kibanamachine kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 11, 2024
eokoneyo pushed a commit to eokoneyo/kibana that referenced this pull request Jan 12, 2024
@PhilippeOberti @eokoneyo
[Security Solution][Timeline] extract and cleanup timeline bottom bar…
df99988 
… component (elastic#173886)
semd pushed a commit to semd/kibana that referenced this pull request Jan 12, 2024
@PhilippeOberti @semd
[Security Solution][Timeline] extract and cleanup timeline bottom bar…
23dbae7 
… component (elastic#173886)
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
@PhilippeOberti @CoenWarmer
[Security Solution][Timeline] extract and cleanup timeline bottom bar…
b937867 
… component (elastic#173886)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janmonschke janmonschke janmonschke approved these changes

@nkhristinin nkhristinin nkhristinin approved these changes

@lgestc lgestc lgestc approved these changes

@tomsonpl tomsonpl tomsonpl approved these changes

@paul-tavares paul-tavares paul-tavares approved these changes

@szwarckonrad szwarckonrad Awaiting requested review from szwarckonrad szwarckonrad is a code owner automatically assigned from elastic/security-defend-workflows

@ferullo ferullo Awaiting requested review from ferullo ferullo is a code owner automatically assigned from elastic/security-defend-workflows

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@PhilippeOberti @janmonschke @kibana-ci @nkhristinin @lgestc @tomsonpl @paul-tavares @kibanamachine