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

[Security Solution][Endpoint][Response Actions] Fix expand details tray #157896

Merged

Conversation

ashokaditya
Copy link
Member

@ashokaditya ashokaditya commented May 16, 2023

Summary

Fixes the bug when sometimes the expanding tray breaks the page on the response actions history page/flyout

It doesn't fix the other issues where when a tray is expanded and paging is used the page breaks.
That is being worked on in /pull/157777

Checklist

Fixes the bug when sometimes the expanding tray breaks the page on response actions history page/flyout
refs elastic/security-team/issues/6603
@ashokaditya ashokaditya self-assigned this May 16, 2023
@ashokaditya ashokaditya added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution OLM Sprint v8.8.0 labels May 16, 2023
@ashokaditya ashokaditya marked this pull request as ready for review May 16, 2023 15:50
@ashokaditya ashokaditya requested a review from a team as a code owner May 16, 2023 15:50
@ashokaditya ashokaditya requested review from pzl and parkiino May 16, 2023 15:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

@kevinlog
Copy link
Contributor

Checked it out and tried it.

I'm able to expand/collapse on Endpoint flyout

image

I'm able to expand/collapse on Response log page

image

@@ -40,6 +40,7 @@ export const useGetEndpointActionList = (
return useQuery<ActionListApiResponse, IHttpFetchError<ErrorType>>({
queryKey: ['get-action-list', query],
...options,
keepPreviousData: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm approving, but am wondering if this has any broader impact?

The docs say this for this option:

If set, any previous data will be kept when fetching new data because the query key changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

One way to test might be to expand the row on the page. Then refresh the browser tab - I think we maintain state in the URL, so on a tab refresh there should be nothing for react-query to keepPreviousData

Copy link
Contributor

Choose a reason for hiding this comment

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

@paul-tavares

I tested this scenario.

  1. Expand rows on page
  2. Refresh browser
  3. See it render as expected with trays expanded from the state in browser

@tomsonpl
Copy link
Contributor

tomsonpl commented May 16, 2023

Hey, I've just played with the issue, and I am not sure if this is the way. I mean, ok to some extend it helps, but the main issue is that we do not have items/action and it crashes at the destructuring.
I think there are some rerendering issues, however the fix would include just waiting for items/action, and showing loader in case it is not ready. Even adding action?. before all fields prevents app from crashing.

What do you think?

@kevinlog
Copy link
Contributor

@tomsonpl

Hey, I've just played with the issue, and I am not sure if this is the way. I mean, ok to some extend it helps, but the main issue is that we do not have items/action and it crashes at the destructuring.
I think there are some rerendering issues, however the fix would include just waiting for items/action, and showing loader in case it is not ready. Even adding action?. before all fields prevents app from crashing.

What do you think?

As we discussed offline, this change in addition to what Ash has already pushed would help further. However, it changes the way the UX around maintaining expanded trays and the overall rendering of the table works, so we'll just merge this change as is since it fixes the problem with expanding trays as we saw earlier.

We'll fix the paging issue with this PR: #157777

@kevinlog kevinlog enabled auto-merge (squash) May 16, 2023 17:31
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 9.2MB 9.2MB +80.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 400 404 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 480 484 +4
total +6

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

cc @ashokaditya

@kevinlog kevinlog merged commit 8210806 into elastic:main May 16, 2023
@ashokaditya ashokaditya deleted the fix/dw-expand-details-tray-bug-partial-fix branch May 16, 2023 17:43
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 16, 2023
…ay (elastic#157896)

## Summary

Fixes the bug when sometimes the expanding tray breaks the page on the
response actions history page/flyout

It doesn't fix the other issues where when a tray is expanded and paging
is used the page breaks.
That is being worked on in elastic/pull/157777

### Checklist

- [ ] [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 8210806)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.8

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 May 16, 2023
…ils tray (#157896) (#157910)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[Security Solution][Endpoint][Response Actions] Fix expand details
tray (#157896)](#157896)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT
[{"author":{"name":"Ashokaditya","email":"1849116+ashokaditya@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-05-16T17:36:54Z","message":"[Security
Solution][Endpoint][Response Actions] Fix expand details tray
(#157896)\n\n## Summary\r\n\r\nFixes the bug when sometimes the
expanding tray breaks the page on the\r\nresponse actions history
page/flyout\r\n\r\nIt doesn't fix the other issues where when a tray is
expanded and paging\r\nis used the page breaks.\r\nThat is being worked
on in /pull/157777\r\n\r\n### Checklist\r\n\r\n- [ ] [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":"82108060e7c3794add5687761cf65890dc39d573","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Defend
Workflows","OLM
Sprint","v8.8.0","v8.9.0"],"number":157896,"url":"https://github.com/elastic/kibana/pull/157896","mergeCommit":{"message":"[Security
Solution][Endpoint][Response Actions] Fix expand details tray
(#157896)\n\n## Summary\r\n\r\nFixes the bug when sometimes the
expanding tray breaks the page on the\r\nresponse actions history
page/flyout\r\n\r\nIt doesn't fix the other issues where when a tray is
expanded and paging\r\nis used the page breaks.\r\nThat is being worked
on in /pull/157777\r\n\r\n### Checklist\r\n\r\n- [ ] [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":"82108060e7c3794add5687761cf65890dc39d573"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/157896","number":157896,"mergeCommit":{"message":"[Security
Solution][Endpoint][Response Actions] Fix expand details tray
(#157896)\n\n## Summary\r\n\r\nFixes the bug when sometimes the
expanding tray breaks the page on the\r\nresponse actions history
page/flyout\r\n\r\nIt doesn't fix the other issues where when a tray is
expanded and paging\r\nis used the page breaks.\r\nThat is being worked
on in /pull/157777\r\n\r\n### Checklist\r\n\r\n- [ ] [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":"82108060e7c3794add5687761cf65890dc39d573"}}]}]
BACKPORT-->

Co-authored-by: Ashokaditya <1849116+ashokaditya@users.noreply.github.com>
jasonrhodes pushed a commit that referenced this pull request May 17, 2023
…ay (#157896)

## Summary

Fixes the bug when sometimes the expanding tray breaks the page on the
response actions history page/flyout

It doesn't fix the other issues where when a tray is expanded and paging
is used the page breaks.
That is being worked on in /pull/157777

### Checklist

- [ ] [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
Labels
OLM Sprint release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.8.0 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants