-
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] show case names in isolation success message #102664
[Security Solution] show case names in isolation success message #102664
Conversation
@elasticmachine merge upstream |
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
cancelCallback: () => void; | ||
}) => { | ||
const [comment, setComment] = useState(''); | ||
const [isIsolated, setIsIsolated] = useState(false); | ||
|
||
const caseIds: string[] = casesInfo.map((caseInfo): string => { |
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.
I think is best to use the useMemo
here and not in caseCount
. The reason is that iterating over an array could be expensive but accessing an array attribute (length
) is not.
@@ -47,7 +52,7 @@ export const IsolateHost = React.memo( | |||
[] | |||
); | |||
|
|||
const caseCount: number = useMemo(() => caseIds.length, [caseIds]); | |||
const caseCount: number = useMemo(() => casesInfo.length, [casesInfo]); |
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.
I think that the overhead of the useMemo
here surpass the benefits using it. const caseCount: number = casesInfo.length
would not cause re-renders as the value is a primitive type.
cancelCallback: () => void; | ||
}) => { | ||
const [comment, setComment] = useState(''); | ||
const [isUnIsolated, setIsUnIsolated] = useState(false); | ||
|
||
const caseIds: string[] = casesInfo.map((caseInfo): string => { |
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.
@@ -75,20 +77,53 @@ export const getCaseIDsByAlertID = async ( | |||
Operations.getCaseIDsByAlertID.savedObjectType | |||
); | |||
|
|||
// This will likely only return one comment saved object, the response aggregation will contain |
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 will likely only return one comment saved object
What will be the response if the same alert is attached to multiple cases?
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 service requests page: 1, perPage: 1
so I think it'll only ever return 1 comment. But the aggregations of that single comment will have all the unique case IDs that we need.
return []; | ||
} | ||
|
||
const casesInfo = await caseService.getCases({ |
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.
Now that we fetch the cases I am wondering if there is any benefit with the aggregation. Do we still need it? The comments will have the case id in the reference
field, right?
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.
Christos and I talked about this offline and we do still need the aggregation because the alert ID could appear in multiple comment saved objects. So I think we're ok to leave this as it is.
💚 Build SucceededMetrics [docs]Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
💔 Backport failed
To backport manually run: |
…stic#102664) # Conflicts: # x-pack/plugins/security_solution/server/endpoint/routes/actions/isolation.ts
Summary
Addresses: #102331
Adds the Case names to the Isolation success message.
Checklist
Delete any items that are not applicable to this PR.