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

[Investigations] - Timeline tabs file cleanup #179832

Merged
merged 91 commits into from
Apr 8, 2024

Conversation

michaelolo24
Copy link
Contributor

@michaelolo24 michaelolo24 commented Apr 2, 2024

Summary

This PR cleans up a bit of the code around timeline tabs. It de-dupes the layout components and some of the shared functionality between the tabs. I would also like to move the tabs from using the connector pattern to using useSelector, but that'll be done in a follow up PR. The commit history unfortunately pulls in a bit from this pr, but the 2 commits for the actual files changed in this PR are as follows:

  1. (No code changes, just moving files) Moving the tabs into a nested tabs folder: 89fa2d8

  2. (Actual Code Changes) De-duping the shared components: c6eecdb

  3. (No code changes, moving filed and renaming) Removed the _content parts of the folder names, and moved the tabs_content files into the tabs folder: ec3b959

@michaelolo24
Copy link
Contributor Author

/ci

@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibana-ci Apr 4, 2024
@michaelolo24
Copy link
Contributor Author

/ci

@PhilippeOberti
Copy link
Contributor

A couple of suggestions to consider:

  • I feel like now that all the tabs are colocated under the same folder, we could rename the _content word from all those folders. It doesn't really provide any value and makes the paths longer...
  • while moving these files around, would it be a good idea to delete the snapshots folders? Looking at the tests I really think these provide no value whatsoever...
  • have you considered moving the new tabs folder on level higher? I understand what it's nested under the timeline folder but I've always hated this sub folder... It's positioned under the timelines/components folder which doesn't really make any sense. And if you look at the components folder above, it has a lot of components used only in the timeline modal and not in the timelines page... I've always wanted to move everything under the components folder then figure out how we differentiate timelines from timeline. I'm not married to this change though so do whatever you want here. I still think though we will need to agree on something at some point because right now it's all over the place!

Side note: I know this PR isn't changing any code which is good, but every time I'm looking at the code in these tab files it makes me so depressed. There are so many usages of EUI flyout related components 😢

@michaelolo24
Copy link
Contributor Author

@PhilippeOberti Thanks for the review! I updated the file names here: ec3b959

  1. I'm not going to remove the snapshot tests quite yet as they have one use case. If someone makes a change that has an unintended effect on any of the tabs with a snapshot currently. I think we can have better tests in the future, but I think that can be done concurrently with adding the unified table logic.

  2. I would move the tabs folder higher, but quite honestly, it feels like it wouldn't change much because as it stands I don't understand why the current files live in the folders they do. I do plan on re-organizing these files to make a little more sense later on, but right now just focusing on the tabs as we'll be working in those directly very shortly.

  3. Tech debt tech debt tech debt. It's not depressing, it's an opportunity to make the app and devex better! 😂

@michaelolo24
Copy link
Contributor Author

/ci

@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@michaelolo24
Copy link
Contributor Author

/ci

@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #24 / Discover alerting Search source Alert should navigate to alert results via link provided in notification using adhoc data view
  • [job] [logs] Jest Tests #12 / Lens Field Item should pass add filter callback and pass result to filter manager
  • [job] [logs] Jest Tests #12 / Lens Field Item should request field stats every time the button is clicked
  • [job] [logs] Jest Integration Tests #1 / migration actions waitForIndexStatus resolves right after waiting for an index status to be yellow if the index already existed

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5296 5298 +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 17.0MB 17.0MB -2.4KB

Page load bundle

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

id before after diff
securitySolution 73.4KB 73.4KB +1.0B

History

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

Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

LGTM for the Threat Hunting Investigations team!

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Reviewing on behalf of the Security Generative AI team -- just three file changes updating imports from the described file moves. LGTM! Thanks @michaelolo24!

@PhilippeOberti PhilippeOberti merged commit b175633 into elastic:main Apr 8, 2024
37 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 8, 2024
michaelolo24 added a commit that referenced this pull request Apr 26, 2024
## Summary

This issue only affects 8.14.

Some wrappers were added unnecessarily to the correlations tab in
Timeline preventing users from scrolling in this PR:
#179832

This PR fixes it by placing the structure more in line with how it was
prior here:
https://github.com/elastic/kibana/pull/179832/files#diff-a11ba69be2253bc1c3183eb6589e3f30b9ec6e77353364208e906526712562fe

The fix:


https://github.com/elastic/kibana/assets/17211684/fc288ab8-3cbf-4f4d-8303-4fb5e970dfcd
michaelolo24 added a commit to michaelolo24/kibana that referenced this pull request Apr 26, 2024
## Summary

This issue only affects 8.14.

Some wrappers were added unnecessarily to the correlations tab in
Timeline preventing users from scrolling in this PR:
elastic#179832

This PR fixes it by placing the structure more in line with how it was
prior here:
https://github.com/elastic/kibana/pull/179832/files#diff-a11ba69be2253bc1c3183eb6589e3f30b9ec6e77353364208e906526712562fe

The fix:

https://github.com/elastic/kibana/assets/17211684/fc288ab8-3cbf-4f4d-8303-4fb5e970dfcd
(cherry picked from commit 11eb39b)

# Conflicts:
#	x-pack/plugins/security_solution/public/timelines/components/timeline/tabs/eql/header/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/timeline/tabs/eql/index.tsx
michaelolo24 added a commit that referenced this pull request Apr 26, 2024
# Backport

This will backport the following commits from `main` to `8.14`:
- [[Bug][Investigations] - Fix eql tab scrolling
(#181763)](#181763)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Michael
Olorunnisola","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-04-26T14:55:43Z","message":"[Bug][Investigations]
- Fix eql tab scrolling (#181763)\n\n## Summary\r\n\r\nThis issue only
affects 8.14.\r\n\r\nSome wrappers were added unnecessarily to the
correlations tab in\r\nTimeline preventing users from scrolling in this
PR:\r\nhttps://github.com//pull/179832\r\n\r\nThis PR
fixes it by placing the structure more in line with how it was\r\nprior
here:\r\nhttps://github.com//pull/179832/files#diff-a11ba69be2253bc1c3183eb6589e3f30b9ec6e77353364208e906526712562fe\r\n\r\nThe
fix:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/17211684/fc288ab8-3cbf-4f4d-8303-4fb5e970dfcd","sha":"11eb39b4b6aa9eada1b7c3e63dc1490762d9d8ea","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Threat
Hunting:Investigations","v8.14.0","v8.15.0"],"number":181763,"url":"https://github.com/elastic/kibana/pull/181763","mergeCommit":{"message":"[Bug][Investigations]
- Fix eql tab scrolling (#181763)\n\n## Summary\r\n\r\nThis issue only
affects 8.14.\r\n\r\nSome wrappers were added unnecessarily to the
correlations tab in\r\nTimeline preventing users from scrolling in this
PR:\r\nhttps://github.com//pull/179832\r\n\r\nThis PR
fixes it by placing the structure more in line with how it was\r\nprior
here:\r\nhttps://github.com//pull/179832/files#diff-a11ba69be2253bc1c3183eb6589e3f30b9ec6e77353364208e906526712562fe\r\n\r\nThe
fix:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/17211684/fc288ab8-3cbf-4f4d-8303-4fb5e970dfcd","sha":"11eb39b4b6aa9eada1b7c3e63dc1490762d9d8ea"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","labelRegex":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/181763","number":181763,"mergeCommit":{"message":"[Bug][Investigations]
- Fix eql tab scrolling (#181763)\n\n## Summary\r\n\r\nThis issue only
affects 8.14.\r\n\r\nSome wrappers were added unnecessarily to the
correlations tab in\r\nTimeline preventing users from scrolling in this
PR:\r\nhttps://github.com//pull/179832\r\n\r\nThis PR
fixes it by placing the structure more in line with how it was\r\nprior
here:\r\nhttps://github.com//pull/179832/files#diff-a11ba69be2253bc1c3183eb6589e3f30b9ec6e77353364208e906526712562fe\r\n\r\nThe
fix:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/17211684/fc288ab8-3cbf-4f4d-8303-4fb5e970dfcd","sha":"11eb39b4b6aa9eada1b7c3e63dc1490762d9d8ea"}}]}]
BACKPORT-->
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 technical debt Improvement of the software architecture and operational architecture v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants