[Security Solution] [Alerts Table] [RAC] Prevent duplicate items being added by bulk actions select all #182007
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kqualters-elastic
added
release_note:skip
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
e40pud
approved these changes
Apr 29, 2024
| @@ -90,7 +92,9 @@ const addItemsToInitialPanel = ({ | |||
| }) => { | |||
| if (panels.length > 0) { | |||
| if (panels[0].items) { | |||
| panels[0].items.push(...items); | |||
| panels[0].items = [...panels[0].items, ...items].filter( | |||
There was a problem hiding this comment.
maybe we could add a comment for curious engineers to understand this logic in future
💚 Build Succeeded
Metrics [docs]Async chunks
To update your PR or re-run it, just comment with: |
cnasikas
approved these changes
Apr 30, 2024
There was a problem hiding this comment.
Thanks for the fix @kqualters-elastic! Code review only. LGTM!
hook calls a function that mutates this array directly, which causes duplicate items to appear in the alert context menu.
Ideally, the helper function should not mutate the original array and return a new array with the new items added.
The test case added below will currently fail on main/8.14.
What do you mean by that?
|
Thanks @cnasikas
That's exactly what I've changed essentially, [...].filter() vs push().
This is just if someone who is reviewing wanted to see the bug, the test passes in this branch with [...].filter() but fails on main when the array is mutated with push() |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Apr 30, 2024
…g added by bulk actions select all (elastic#182007) ## Summary Related issue: elastic#181972 When the useBulkActions hook in the alerts table is used with the optionally defined at registration time hook useBulkActionsConfig, and that hook returns a stable array, the hook calls a function that mutates this array directly, which causes duplicate items to appear in the alert context menu. This is due to the useBulkActions hook using the bulk actions state via context, and so runs every time a user selects a row/all rows. Doing this via filter might not be the most performant way, but in order to have everything referentially stable, I think the arguments to useBulkActions/useBulkUntrackActions should be changed a bit, but that can be a discussion for another time. The test case added below will currently fail on main/8.14. Before:  After:  ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit 8a1d295)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Apr 30, 2024
…ms being added by bulk actions select all (#182007) (#182109) # Backport This will backport the following commits from `main` to `8.14`: - [[Security Solution] [Alerts Table] [RAC] Prevent duplicate items being added by bulk actions select all (#182007)](#182007) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kevin Qualters","email":"56408403+kqualters-elastic@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-04-30T12:11:43Z","message":"[Security Solution] [Alerts Table] [RAC] Prevent duplicate items being added by bulk actions select all (#182007)\n\n## Summary\r\n\r\nRelated issue: #181972\r\n\r\nWhen the useBulkActions hook in the alerts table is used with the\r\noptionally defined at registration time hook useBulkActionsConfig, and\r\nthat hook returns a stable array, the hook calls a function that mutates\r\nthis array directly, which causes duplicate items to appear in the alert\r\ncontext menu. This is due to the useBulkActions hook using the bulk\r\nactions state via context, and so runs every time a user selects a\r\nrow/all rows. Doing this via filter might not be the most performant\r\nway, but in order to have everything referentially stable, I think the\r\narguments to useBulkActions/useBulkUntrackActions should be changed a\r\nbit, but that can be a discussion for another time. The test case added\r\nbelow will currently fail on main/8.14.\r\n\r\nBefore:\r\n\r\n\r\n\r\n\r\nAfter:\r\n\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"8a1d2950fa16f8cb50a21cb793640a290b54032c","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","Team:Threat Hunting:Investigations","v8.14.0","v8.15.0"],"title":"[Security Solution] [Alerts Table] [RAC] Prevent duplicate items being added by bulk actions select all","number":182007,"url":"#182007 Solution] [Alerts Table] [RAC] Prevent duplicate items being added by bulk actions select all (#182007)\n\n## Summary\r\n\r\nRelated issue: #181972\r\n\r\nWhen the useBulkActions hook in the alerts table is used with the\r\noptionally defined at registration time hook useBulkActionsConfig, and\r\nthat hook returns a stable array, the hook calls a function that mutates\r\nthis array directly, which causes duplicate items to appear in the alert\r\ncontext menu. This is due to the useBulkActions hook using the bulk\r\nactions state via context, and so runs every time a user selects a\r\nrow/all rows. Doing this via filter might not be the most performant\r\nway, but in order to have everything referentially stable, I think the\r\narguments to useBulkActions/useBulkUntrackActions should be changed a\r\nbit, but that can be a discussion for another time. The test case added\r\nbelow will currently fail on main/8.14.\r\n\r\nBefore:\r\n\r\n\r\n\r\n\r\nAfter:\r\n\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"8a1d2950fa16f8cb50a21cb793640a290b54032c"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"#182007 Solution] [Alerts Table] [RAC] Prevent duplicate items being added by bulk actions select all (#182007)\n\n## Summary\r\n\r\nRelated issue: #181972\r\n\r\nWhen the useBulkActions hook in the alerts table is used with the\r\noptionally defined at registration time hook useBulkActionsConfig, and\r\nthat hook returns a stable array, the hook calls a function that mutates\r\nthis array directly, which causes duplicate items to appear in the alert\r\ncontext menu. This is due to the useBulkActions hook using the bulk\r\nactions state via context, and so runs every time a user selects a\r\nrow/all rows. Doing this via filter might not be the most performant\r\nway, but in order to have everything referentially stable, I think the\r\narguments to useBulkActions/useBulkUntrackActions should be changed a\r\nbit, but that can be a discussion for another time. The test case added\r\nbelow will currently fail on main/8.14.\r\n\r\nBefore:\r\n\r\n\r\n\r\n\r\nAfter:\r\n\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"8a1d2950fa16f8cb50a21cb793640a290b54032c"}}]}] BACKPORT--> Co-authored-by: Kevin Qualters <56408403+kqualters-elastic@users.noreply.github.com>
yuliacech
pushed a commit
to yuliacech/kibana
that referenced
this pull request
May 3, 2024
…g added by bulk actions select all (elastic#182007) ## Summary Related issue: elastic#181972 When the useBulkActions hook in the alerts table is used with the optionally defined at registration time hook useBulkActionsConfig, and that hook returns a stable array, the hook calls a function that mutates this array directly, which causes duplicate items to appear in the alert context menu. This is due to the useBulkActions hook using the bulk actions state via context, and so runs every time a user selects a row/all rows. Doing this via filter might not be the most performant way, but in order to have everything referentially stable, I think the arguments to useBulkActions/useBulkUntrackActions should be changed a bit, but that can be a discussion for another time. The test case added below will currently fail on main/8.14. Before:  After:  ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Related issue: #181972
When the useBulkActions hook in the alerts table is used with the optionally defined at registration time hook useBulkActionsConfig, and that hook returns a stable array, the hook calls a function that mutates this array directly, which causes duplicate items to appear in the alert context menu. This is due to the useBulkActions hook using the bulk actions state via context, and so runs every time a user selects a row/all rows. Doing this via filter might not be the most performant way, but in order to have everything referentially stable, I think the arguments to useBulkActions/useBulkUntrackActions should be changed a bit, but that can be a discussion for another time. The test case added below will currently fail on main/8.14.
Before:
After:
Checklist