-
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
[Cloud Posture]fix incorrect vuln_mgmt details in flyout #157745
[Cloud Posture]fix incorrect vuln_mgmt details in flyout #157745
Conversation
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
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.
Checked locally, LGTM
const selectedVulnerability = data?.page[urlQuery.vulnerabilityIndex]; | ||
|
||
const selectedVulnerability = useMemo(() => { | ||
return slicedPage?.find( |
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.
@Omolola-Akinleye why we use find if we know the index?
shouldn't slicedPage[urlQuery.vulnerabilityIndex] would work here? and would be O(1) instead of O(n)
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.
Fair point. Typescript throw undefined when I was using data?.page
. I incorrectly confused slicedPage for data.page
as being possibly undefined. I will adjust that.
@@ -116,7 +116,10 @@ const VulnerabilitiesContent = ({ dataView }: { dataView: DataView }) => { | |||
const slicedPage = usePageSlice(data?.page, pageIndex, pageSize); | |||
|
|||
const invalidIndex = -1; | |||
const selectedVulnerability = data?.page[urlQuery.vulnerabilityIndex]; | |||
|
|||
const selectedVulnerability = useMemo(() => { |
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.
can you explain why useMemo is needed here? if the calculation is O(1), I assume it is redundant overhead
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.
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.
@kfirpeled Good point.useMemo
is not needed here. It's over-optimization
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
) ## Summary Summarize your PR. If it involves visual changes include a screenshot or gif. Currently, after navigating any page after the first page and clicking on vulnerability, the flyout displays incorrect details. Currently, we use the `data.page`, which lists the maximum amount of vulnerabilities(500 vulnerabilities). The `vulnerabilityIndex` is set each time, a user clicks to expand vulnerability. We were currently only selecting vulnerabilities from the first page. Solution: Use `slicedPage` to get the vulnerabilities on the current page and then select vulnerability by `vulnerabilityIndex` https://github.com/elastic/kibana/assets/17135495/df682ecf-d024-45e4-84ca-d7df22a60ec4 (cherry picked from commit 5eb9102)
…) (#157795) # Backport This will backport the following commits from `main` to `8.8`: - [[Cloud Posture]fix incorrect vuln_mgmt details in flyout (#157745)](#157745) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Lola","email":"omolola.akinleye@elastic.co"},"sourceCommit":{"committedDate":"2023-05-15T19:48:22Z","message":"[Cloud Posture]fix incorrect vuln_mgmt details in flyout (#157745)\n\n## Summary\r\n\r\nSummarize your PR. If it involves visual changes include a screenshot or\r\ngif.\r\nCurrently, after navigating any page after the first page and clicking\r\non vulnerability, the flyout displays incorrect details.\r\n\r\nCurrently, we use the `data.page`, which lists the maximum amount of\r\nvulnerabilities(500 vulnerabilities). The `vulnerabilityIndex` is set\r\neach time, a user clicks to expand vulnerability. We were currently only\r\nselecting vulnerabilities from the first page.\r\n\r\nSolution:\r\n\r\nUse `slicedPage` to get the vulnerabilities on the current page and then\r\nselect vulnerability by `vulnerabilityIndex`\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/17135495/df682ecf-d024-45e4-84ca-d7df22a60ec4","sha":"5eb9102bc9313ff46ae72bb6af71e87e09c2ad7c","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Cloud Security","v8.8.0","v8.9.0"],"number":157745,"url":"https://github.com/elastic/kibana/pull/157745","mergeCommit":{"message":"[Cloud Posture]fix incorrect vuln_mgmt details in flyout (#157745)\n\n## Summary\r\n\r\nSummarize your PR. If it involves visual changes include a screenshot or\r\ngif.\r\nCurrently, after navigating any page after the first page and clicking\r\non vulnerability, the flyout displays incorrect details.\r\n\r\nCurrently, we use the `data.page`, which lists the maximum amount of\r\nvulnerabilities(500 vulnerabilities). The `vulnerabilityIndex` is set\r\neach time, a user clicks to expand vulnerability. We were currently only\r\nselecting vulnerabilities from the first page.\r\n\r\nSolution:\r\n\r\nUse `slicedPage` to get the vulnerabilities on the current page and then\r\nselect vulnerability by `vulnerabilityIndex`\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/17135495/df682ecf-d024-45e4-84ca-d7df22a60ec4","sha":"5eb9102bc9313ff46ae72bb6af71e87e09c2ad7c"}},"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/157745","number":157745,"mergeCommit":{"message":"[Cloud Posture]fix incorrect vuln_mgmt details in flyout (#157745)\n\n## Summary\r\n\r\nSummarize your PR. If it involves visual changes include a screenshot or\r\ngif.\r\nCurrently, after navigating any page after the first page and clicking\r\non vulnerability, the flyout displays incorrect details.\r\n\r\nCurrently, we use the `data.page`, which lists the maximum amount of\r\nvulnerabilities(500 vulnerabilities). The `vulnerabilityIndex` is set\r\neach time, a user clicks to expand vulnerability. We were currently only\r\nselecting vulnerabilities from the first page.\r\n\r\nSolution:\r\n\r\nUse `slicedPage` to get the vulnerabilities on the current page and then\r\nselect vulnerability by `vulnerabilityIndex`\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/17135495/df682ecf-d024-45e4-84ca-d7df22a60ec4","sha":"5eb9102bc9313ff46ae72bb6af71e87e09c2ad7c"}}]}] BACKPORT--> Co-authored-by: Lola <omolola.akinleye@elastic.co>
## Summary Summarize your PR. If it involves visual changes include a screenshot or gif. Currently, after navigating any page after the first page and clicking on vulnerability, the flyout displays incorrect details. Currently, we use the `data.page`, which lists the maximum amount of vulnerabilities(500 vulnerabilities). The `vulnerabilityIndex` is set each time, a user clicks to expand vulnerability. We were currently only selecting vulnerabilities from the first page. Solution: Use `slicedPage` to get the vulnerabilities on the current page and then select vulnerability by `vulnerabilityIndex` https://github.com/elastic/kibana/assets/17135495/df682ecf-d024-45e4-84ca-d7df22a60ec4
Summary
Summarize your PR. If it involves visual changes include a screenshot or gif.
Currently, after navigating any page after the first page and clicking on vulnerability, the flyout displays incorrect details.
Currently, we use the
data.page
, which lists the maximum amount of vulnerabilities(500 vulnerabilities). ThevulnerabilityIndex
is set each time, a user clicks to expand vulnerability. We were currently only selecting vulnerabilities from the first page.Solution:
Use
slicedPage
to get the vulnerabilities on the current page and then select vulnerability byvulnerabilityIndex
Updated.mov