-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Detections Engine] Add Alert actions to the Timeline #73228
[Detections Engine] Add Alert actions to the Timeline #73228
Conversation
…tions-timeline # Conflicts: # x-pack/plugins/security_solution/public/app/app.tsx # x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx # x-pack/plugins/security_solution/public/detections/components/alerts_table/default_config.tsx # x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx # x-pack/plugins/security_solution/public/detections/pages/detection_engine/detection_engine.tsx # x-pack/plugins/security_solution/public/hosts/pages/navigation/events_query_tab_body.tsx # x-pack/plugins/security_solution/public/timelines/components/open_timeline/helpers.test.ts # x-pack/plugins/security_solution/public/timelines/components/timeline/body/helpers.tsx # x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.test.tsx # x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.tsx # x-pack/plugins/security_solution/public/timelines/components/timeline/body/stateful_body.tsx
@elasticmachine merge upstream |
Pinging @elastic/siem (Team:SIEM) |
@elasticmachine merge upstream |
content?: string; | ||
iconType?: string; | ||
isDisabled?: boolean; | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any |
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 linter can be re-enabled by changing the import of React
to include the following:
import React, { MouseEvent } from 'react';
and changing
onClick?: any;
to
onClick?: (event: MouseEvent) => void;
That passes the type checker:
➜ kibana git:(feat/alert-actions-timeline) ✗ node scripts/type_check.js --project x-pack/tsconfig.json
✔ x-pack
</EventsTdGroupActions> | ||
); | ||
}, | ||
(nextProps, prevProps) => { |
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.
Thanks for removing this touchpoint, which can be the source of subtle bugs. No action is required for this PR, but it probably makes sense to (manually) test for unnecessary re-renders after the timeline query has been migrated to the "search strategy".
…tions-timeline # Conflicts: # x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx # x-pack/plugins/security_solution/public/timelines/components/manage_timeline/index.tsx
…tions-timeline # Conflicts: # x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx
…tions-timeline # Conflicts: # x-pack/plugins/security_solution/public/timelines/pages/timelines_page.tsx
@elasticmachine merge upstream |
const onAlertStatusUpdateSuccess = useCallback( | ||
(count: number, newStatus: Status) => { | ||
let title: string; | ||
switch (newStatus) { |
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.
Consider adding a default
case or an exhaustive check to avoid issues where title
is undefined
because a new Status
is added.
const onAlertStatusUpdateFailure = useCallback( | ||
(newStatus: Status, error: Error) => { | ||
let title: string; | ||
switch (newStatus) { |
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.
Consider adding a default
case or an exhaustive check to avoid issues where title
is undefined
because a new Status
is added.
const openAlertActionComponent = ( | ||
<EuiContextMenuItem | ||
key="open-alert" | ||
aria-label="Open alert" |
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.
consider replacing this text with i18n.ACTION_OPEN_ALERT
const closeAlertActionComponent = ( | ||
<EuiContextMenuItem | ||
key="close-alert" | ||
aria-label="Close alert" |
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.
consider replacing this with i18n.ACTION_CLOSE_ALERT
const inProgressAlertActionComponent = ( | ||
<EuiContextMenuItem | ||
key="in-progress-alert" | ||
aria-label="Mark alert in progress" |
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.
consider replacing this with i18n.ACTION_IN_PROGRESS_ALERT
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
History
To update your PR or re-run it, just comment with: |
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.
Thanks for this feature and the refactoring @patrykkopycinski! 🙏
Desk tested timeline & timeline-based views locally
LGTM 🚀
* master: (223 commits) skip flaky suite (elastic#75724) [Reporting] Add functional test for Reports in non-default spaces (elastic#76053) [Enterprise Search] Fix various icons in dark mode (elastic#76430) skip flaky suite (elastic#76245) Add `auto` interval to histogram AggConfig (elastic#76001) [Resolver] generator uses setup_node_env (elastic#76422) [Ingest Manager] Support both zip & tar archives from Registry (elastic#76197) [Ingest Manager] Improve agent vs kibana version checks (elastic#76238) Manually building `KueryNode` for Fleet's routes (elastic#75693) remove dupe tinymath section (elastic#76093) Create APM issue template (elastic#76362) Delete unused file. (elastic#76386) [SECURITY_SOLUTION][ENDPOINT] Trusted Apps Create API (elastic#76178) [Detections Engine] Add Alert actions to the Timeline (elastic#73228) [Dashboard First] Library Notification (elastic#76122) [Maps] Add mvt support for ES doc sources (elastic#75698) Add setHeaderActionMenu API to AppMountParameters (elastic#75422) [ML] Remove "Are you sure" from data frame analytics jobs (elastic#76214) [yarn] remove typings-tester, use @ts-expect-error (elastic#76341) [Reporting/CSV] Do not fail the job if scroll ID can not be cleared (elastic#76014) ...
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
additionalActions
from config object to React componentstimelineRowActions
fromManageTimelineContext
in favor of alert type based logicaddExceptionModal
logic to the separate componenttimeline-1
with enumTimelineId.active
KNOWN ISSUE: Timeline results don't refresh after the alert status change, will be fixed after #75591 is merged
Checklist