-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security Solution] Integrate Vanilla Unified Data Table in Timeline #176064
[Security Solution] Integrate Vanilla Unified Data Table in Timeline #176064
Conversation
2a6c95c
to
258ed5d
Compare
x-pack/plugins/security_solution/public/common/components/sourcerer/get_sourcerer_data_view.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/common/lib/cell_actions/field_value.tsx
Outdated
Show resolved
Hide resolved
...n/public/detections/components/alerts_table/timeline_actions/use_investigate_in_timeline.tsx
Show resolved
Hide resolved
updatedAt: number; | ||
} | ||
|
||
export const FixedWidthLastUpdatedContainer = React.memo<FixedWidthLastUpdatedContainerProps>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder for @logeekal : need to check why is this needed when there is a last updated component already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YulNaumenko do you remember?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separated out and deduped last updated component. This is the final one with tests : https://github.com/elastic/kibana/pull/176064/files#diff-36743070b5744453188c3858a3869fa054832cba119926b371108f818cb01a77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the commit : 4c8fe91
@@ -37,6 +37,12 @@ describe( | |||
{ product_line: 'security', product_tier: 'essentials' }, | |||
{ product_line: 'endpoint', product_tier: 'essentials' }, | |||
], | |||
// alertSuppressionForIndicatorMatchRuleEnabled feature flag is also enabled in a global config | |||
kbnServerArgs: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the feature flag has been added globally? We should add FF at the beginning of the spec file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It must be a merge issue. Let me check.. I did not touch this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added as part of this PR. I think while resolving merge conflicts some issue happenned. I will just correct this in accordance with main. Additionally, I could not find this flag in global config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MadameSheema , I have restored this file according to main.
context('flyout', () => { | ||
it('should be able to open/close details details/host/user flyout', () => { | ||
cy.log('Event Details Flyout'); | ||
openEventDetailsFlyout(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the assertion you are doing in the openEventDetailsFlyout
I'll add it here to make more visible what is happening, if not seems that we are doing a test without assertions :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed here: 84fe0fb
@@ -112,6 +112,10 @@ export const addFieldToTable = (fieldId: string) => { | |||
clearFieldSearch(); | |||
}; | |||
|
|||
export const removeFieldFromTable = (fieldId: string) => { | |||
cy.get(GET_DISCOVER_COLUMN_TOGGLE_BTN(fieldId)).first().trigger('click'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using .trigger('click')
instead of click()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed here: 84fe0fb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of thanks for addressing all the comments! :)
@@ -254,13 +251,21 @@ export const getTimelineStatus = ( | |||
export const defaultTimelineToTimelineModel = ( | |||
timeline: TimelineResult, | |||
duplicate: boolean, | |||
timelineType?: TimelineType | |||
timelineType?: TimelineType, | |||
unifiedComponentsInTimelineEnabled?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current approach forces many components to retrieve this value using useIsExperimentalFeatureEnabled
to pass it to this function.
Wouldn't it be easier to use ExperimentalFeaturesService
singleton inside this file, instead of receiving this value by parameter? It would simplify the cleaning when we remove this flag too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@semd , I did not even know that it existed. I agree with you but I have three more upcoming PRs for this functionality. Do you mind, if I do that change in next PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sure, it's just a simplification, and also to reduce the scope of the changes. But if you keep it this way it's also fine, as you prefer 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I will have to check the Singleton class API but I will make changes accordingly in the next PR.
I would be glad as well if I could remove this extra param from everywhere.
I hope we go back to previous sizes when we clean the old version. Thanks for doing these changes, looks great 💯 |
/ci |
/ci |
1 similar comment
/ci |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @logeekal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few observations:
- in the PR description, we say that this PR fixes this ticket but looking at the tasks on that ticket, a lot of them aren't actually covered by this PR. We should maybe remove the link to the ticket so it doesn't confuse PR later?
- PR description point 2: the expandable flyout isn't opening, even when using the
expandableTimelineFlyoutEnabled
flag. This is valid for the document details, host and user flyouts. For all of them, only the old flyout opens (but it does open on top of timeline instead of within the timeline modal along side the table so that's good) - there is a weird blinking happening when removing a column. Also I wonder why we fetch the data on removing columns, it's not like we need new data...
https://github.com/elastic/kibana/assets/17276605/5fb47ce8-a536-4ea3-b915-6217ab49bbd6 - PR description point 3: I'm confused about that the
+
andx
buttons are? - I'm wondering why we trigger a fetch for the data when we move a column around. It's not like we need any new information?
- Same thing for the column sorting. Are we fetching different data that the 500 (or whatever selected in the table control display option) or just sorting the data in the frontend?
- It seems that sorting by
@timestamp
isn't working. Other columns seem to be working fine
https://github.com/elastic/kibana/assets/17276605/fbca15a3-3212-4a97-bbb1-0ad38b510d84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for the Threat Hunting Investigations team, as all the points are raised will be tackled in a subsequent PR (see issue)
@logeekal @michaelolo24 I think it would be nice to update the PR description to really reflect what the code in this PR does, and what it does now. I raised a few points in my last comment, but I think having the PR description as reflective of the code as possible will be beneficiary if we come back to this one day. |
## 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](#176064), 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 --------- Co-authored-by: Jatin Kathuria <jatin.kathuria@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Fixes below issues
Details
Objective
The objective of this PR is to implement basic unified data table replacing the current timeline table. It is intended to behave exactly like discover table. Below are the functionalities of the timeline table that are out of scope of this PR and will be included in the follow up PR.
after_timeline_unified.mov
before_timeline_unified.mov
This feature can be enabled with below feature flag:
Out of scope functionalities
Desk Testing Guide
Below are some areas which have changes and would warrant some desk testing.
One pre-requisite is to enable feature flag
unifiedComponentsInTimelineEnabled
which will replace traditional timeline table with the new unified timeline table.Load More
on-demand.newold Expandable flyout - ( New Expandable flCurrent Observations of Unified table vis-a-vis discover UI + Issues
Checklist
Delete any items that are not applicable to this PR.