cisco-sbg-ui / magna-react

magna-react.vercel.app
5 stars 0 forks source link

APagination without jumping to a specific (or the last) page #113

Closed mvelkCisco closed 1 year ago

mvelkCisco commented 1 year ago

While migrating Judgmets table in Intelligence application to magna-react, we have realized that APagination is not suitable for us (or for usage with CTIA and a lot of data in general).

The original implementation in Intelligence:

image

Closest magna-react implementation:

image

The API does not allow us to jump to a specific page (including the last page).

Describe the solution you'd like

There seem to be hundreds of millions of judgments. So we would like to:

There doesn't seem to be such a variant in magna-react.

This version was approved by @derrick-snider:

image

Describe alternatives you've considered

Initial issue was that Judgements uses graphql API, which does not support jumping to a specific page number as we need to know the cursor.

I did some digging and it also doesn't support jumping to the last page (even though GQL in general can do that, the CTIA implementation just maps before/after/cursor to limit/offset https://github.com/threatgrid/ctia/blob/master/src/ctia/schemas/graphql/pagination.clj#L64) and returns error if there's before but no cursor specified.

I tried switching to REST limit/offset pagination instead to avoid the cursor issue, and found out that REST (and transitively also graphql) does not allow offset pagination after 10,000th item (I'm hitting max_result_window limit). It's recommend to use cursor pagination for more data https://github.com/threatgrid/ctia#stateless-cursor-pagination, so again jumping to a specific page is not possible.

Also, the REST API supports only paginating forward, not backward (search_after in https://private.intel.amp.cisco.com/index.html#/Judgement/get_ctia_judgement_search , but no search_before), which again prevents implementing jumping to the last page when doing cursor pagination with REST.

Additional context

I assume other teams relying on CTIA will run into the same issue.

For now I've implemented some ugly overrides in css to produce the proposal above: https://github.com/advthreat/intelligence/pull/38/commits/7cd12cc421c3baa03e3dfbd2d0240835c1303515

The pagination in magna-react doesn't match magnetic very closely, there seems to be only 1 valid variant: https://www.figma.com/file/oVZWatImEIbl1c8sjdGxi0/%F0%9F%A7%B2--Magnetic-Design-Library?node-id=5247%3A39676&t=R5YrG947AWcqIDjU-0

image

I think they've actually designed this with cursor pagination in mind, because they show going only 1 page forward or back (in contrast to magna-react). However they also show the link to the last page, which would currently now work with neither graphql (not implemented, returns error) nor REST (can use cursor only for going forwards, not backwards).

rwharrisjr commented 1 year ago

Adding two props on APagination:

These should allow for customization to match the approved design and handle non-cursor backed results.

Screenshot 2023-01-23 at 4 25 26 PM

sabrinamokerji commented 1 year ago

@rwharrisjr shouldn't the first |< and last >| arrows be removed when page jumping is disabled?

rwharrisjr commented 1 year ago

@sabrinamokerji yes, I forgot to update the doc page after I changed the prop name in the code :/