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.

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

Expandable flyout context #165662

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented Sep 5, 2023

Summary

This PR performs a small cleanup on the multiple contexts we've developed for the SecuritySolution expandable flyout:

  • extract the error logic into its own component
  • extract the loading logic into its own component
  • extract the event details data retrieval into its own hook
  • memoize the providers (which seems to fix the flickering when opening the flyout)
  • make sure that all providers always return non null/undefined data

This change ensures that all components within the flyout will always receive non null/undefined data. This allows to remove a lot of null checks and to remove a bunch of unnecessary unit tests.

The decision was made to NOT combine all the providers into one, as the SecuritySolution flyout is intended to see more panels added, some of them not related to an alert document. Therefore, sharing the current information will not make sense at all. We're keeping a little bit of code duplication, but the extraction of the error and loading components, as well as the event details hook reduces that to a very small amount.

Loading and showing data

Screen.Recording.2023-09-11.at.11.22.33.AM.mov

Showing error for one or multiple panels at the same time

Screen.Recording.2023-09-11.at.11.23.36.AM.mov
Screen.Recording.2023-09-11.at.11.25.20.AM.mov

https://github.com/elastic/security-team/issues/7465
https://github.com/elastic/security-team/issues/6793

Checklist

@PhilippeOberti PhilippeOberti force-pushed the expandable-flyout-context branch 2 times, most recently from 7769755 to 81627ab Compare September 11, 2023 09:29
@PhilippeOberti PhilippeOberti marked this pull request as ready for review September 11, 2023 11:32
@PhilippeOberti PhilippeOberti requested a review from a team as a code owner September 11, 2023 11:32
@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.11.0 labels Sep 11, 2023
Copy link
Contributor

@christineweng christineweng left a comment

Choose a reason for hiding this comment

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

@PhilippeOberti great work! Code looks more clean and consistent across all the panels and tabs 👍 I left some nit comments (mostly just questions)

With regards to combining vs separating the context, I do feel that sharing commonly used contexts like scopeId, index etc. could benefit all the panels. Another route instead of all or nothing, is to consider using the shared folder. There are already some duplicates with the mock_context folder (it is present in left, right and shared folders).

With that being said, no actions for this particular PR, just throwing some thoughts while we are refactoring 😃

@PhilippeOberti PhilippeOberti mentioned this pull request Sep 12, 2023
1 task
@PhilippeOberti PhilippeOberti force-pushed the expandable-flyout-context branch 4 times, most recently from 0e25510 to fd20076 Compare September 13, 2023 11:19
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4510 4512 +2

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.6MB 12.6MB -3.2KB
Unknown metric groups

References to deprecated APIs

id before after diff
securitySolution 652 654 +2

History

  • 💔 Build #158638 failed fd20076568f83947937cc50e6cccf84e7565c437
  • 💔 Build #158579 failed 0e2551039002dd2a5bed557b23cab86fff65b896
  • 💔 Build #158563 failed 57d3dfd11b147fdab58c236858a0c1d09cbc696c
  • 💔 Build #157906 failed 81627ab0c4650abeb62be4a2b94f6c4bc3328a16
  • 💔 Build #156313 failed 5cdf09afbe32410c4f9586402c47068eef90a923

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

@PhilippeOberti
Copy link
Contributor Author

@PhilippeOberti great work! Code looks more clean and consistent across all the panels and tabs 👍 I left some nit comments (mostly just questions)

With regards to combining vs separating the context, I do feel that sharing commonly used contexts like scopeId, index etc. could benefit all the panels. Another route instead of all or nothing, is to consider using the shared folder. There are already some duplicates with the mock_context folder (it is present in left, right and shared folders).

With that being said, no actions for this particular PR, just throwing some thoughts while we are refactoring 😃

Thanks for the thorough review @christineweng! I fixed all the things you noticed.

Regarding the context separation topic, I think for now I'd like to keep them as is. The code duplication is small (thanks to the new components and hook). I'm not sure there are items that I would consider as common: indexName and eventId wouldn't make sense if we have a new flyout panel for a rule for example, or a host... I think when other teams start adding new panels, if we see that something really stands out as common we'll extract it.

@PhilippeOberti PhilippeOberti merged commit 170024d into elastic:main Sep 13, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 13, 2023
@PhilippeOberti PhilippeOberti deleted the expandable-flyout-context branch September 13, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants