[Cloud Posture] add pagination to findings by resource

[orouz]

Summary

adds pagination to group-by-resource table

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

[elasticmachine]

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security Posture)

[amitkanfer]

@orouz - aren't we missing a counter somewhere to show "page 1 of many", "page 2 of .."
I know we don't know how many pages we have, but seems weird that we don't have an indication at all...

Is this how it works in other places in Kibana?

[kfirpeled]

@orouz - aren't we missing a counter somewhere to show "page 1 of many", "page 2 of .." I know we don't know how many pages we have, but seems weird that we don't have an indication at all...

Is this how it works in other places in Kibana?

Hey @amitkanfer , the current pagination is what comes out of the box also in EUITablePagination. Because we are in a tight schedule I'll postpone that tiny fix from this PR. lets do that after our internal FF.
Another note on @orouz work, he went over and did his own pagination component in order to remove the first/last pages as well. Since we can't show the last one.
I'm reaching eui team for more details about that

[ari-aviran]

I'm rejecting before actually going over the code, some of the page's functionality is not working as expected.

The "rows per page" break the pagination. I think "rows per page" can never work with cursor based pagination, as changing the size of the page will always break backward navigation (if you are on page 5 and you change page size from 10 to 20 you don't have the cursor to fetch the previous 20, only the previous 10 items, and you end up with documents showing up in multiple pages). Here's a short video demonstrating an issue, after changing the page size there's no way to see previous findings

Screen.Recording.2022-04-28.at.11.48.56.mov

I suggest removing the rows per page functionality completely, and setting it to 20.

In addition, persisting the page in the URL is also problematic (and was explicitly out of scope for the task). The issue is that when a user refreshes the page, they don't have the previous cursors in memory so the back navigation always takes you to the first page, which is very confusing UX. This also can not be implemented properly, even if you somehow store previous cursors between page refreshes (e.g. local storage or similar) since new data might have arrived that messes with the order of documents, making the previous cursors invalid.

Here's a video demonstrating the issue:

Screen.Recording.2022-04-28.at.11.49.36.mov

[ari-aviran]

Here's a video that better describes the "rows per page" issue I mentioned with duplicate data between pages:

111.mov

[orouz]

I'm rejecting before actually going over the code, some of the page's functionality is not working as expected.

agreed, there's some funky business here around pagination (some messy code too) 😄

The "rows per page" break the pagination

yes, seems like a bug. i think whenever it changes, the after_keys should reset so we start a clean slate.
I prefer not removing the selection of how many rows to show, as that diverges the pagination between the two tables even further (now only the right side is different). will fix

persisting the page in the URL is also problematic

yes, this is known. complete seperation of queries will happen (as it's required) in my next PR

regarding the UX issue, as you've noticed, it's a best effort. but i do think it has some sense:

• initial visits without an after_key in the URL will results in the back button being disabled, as we can't go back.
• initial visits with an after_key will have a back button enabled, so users can go back to the first page.

this is very much confusing, but preventing them from ever getting to the first page would probably be weirder. and like you said, there is no reliable way to know the previous cursors. we could probably mitigate this with a callout whenever users enters the page with an after key on initial visit, making sure they know "back" means "first" in this case.

---

^{_{this PR has a high ratio. 😅}}

[ari-aviran]

Overall LGTM other than the bug I commented about. Please also reply to @JordanSh comments

[ari-aviran]

This is an error, it displays wrong number of failed findings, I believe this should be reverted to bucket.failed_findings.doc_count

Video:

Screen.Recording.2022-05-17.at.11.46.22.mov

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 033805e
• Storybooks Preview
• Webpack Bundle Analyzer

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
cloudSecurityPosture	182.4KB	183.0KB	+633.0B

History

• 💚 Build #44827 succeeded 9d2e3729e6f72c0daa7585e2f07c470d86af1ea1
• 💚 Build #44737 succeeded 35e32ccf5cdeb0d2d6280419c67deb92efbca27b
• 💔 Build #44729 failed d8c24069df7daf652fa22bfbd09602d1fd09b5da
• 💚 Build #42173 succeeded e91811b94af54065d6cf1fb0ef4427bd0d918449
• 💚 Build #41641 succeeded 838c09704d74c09f1470678dba67f245e3b2c909
• 💚 Build #40953 succeeded b4dea7e

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

[ari-aviran]

Consider using await instead of then

[JordanSh]

+1

[orouz]

will do in the next one (overlaps anyway) and i'd prefer merge this now