Fixes #33276 - Improve TableWrapper change tracking for search & pagination

[jeremylenz]

The spawnFetch function within TableWrapper tracks several changes and prevents sending duplicate API requests for search and pagination. The changes tracked are

• prevRequest - Don't send a request if it has the same metadata as the previously-sent request
• paginationChangePending - Don't send a request with stale data if the user has clicked on a pagination button and the request is still in flight
• prevSearch - Don't send duplicate requests with the same search query
• additionalListeners - Send a new request if additionalListeners has changed, even if all of the above conditions would otherwise prevent it
• Also, pagination params are supposed to reset back to page 1 whenever search or active filters change.

activeFilters was supposed to be tracking changes, but it wasn't. This caused pagination to be reset to page 1 when it shouldn't be.

Also, paginationChangePending wasn't getting reset when it should be.

What are the changes introduced in this pull request?

• Change activeFilters from a boolean prop to an array of strings
• Use a ref to track changes in activeFilters
• Add variable names to spawnFetch so that logic is more readable
• Clear paginationChangePending when resetting to page 1
• Clear paginationChangePending before triggering a user pagination change (onPaginationChange)
• Send the request through if additionalListeners have changed, even if stale pagination data would otherwise prevent it

What are the testing steps for this pull request?

You may want to check out the first commit, which has a lot of useful console logs added.

Load a page with filters, such as the Content Views tab of a composite content view
Change to page 2 > this should respond on the first click
Change filter from 'All' to 'Not added' > this should respond normally and not cause an infinite loop
Change filter from 'Not added' to 'Added' > this should respond normally and not cause an infinite loop
also make sure changing per_page works everywhere

[theforeman-bot]

Issues: #33276

[jeremylenz]

one remaining bug: Set perPage to 10 (or a non-default value) on the Filters tab, go to Histories, set a perPage, then go back to Filters. This would sometimes cause an infinite API call loop, with the per_page alternating between the default 20 and whatever you set it to.

I think this is caused by pages that store metadata in state. I removed metadata from state and it seems to fix it, so I went ahead and removed it from all other pages that store metadata in state as well.

[jeremylenz]

Updated and rebased:

• extracted metadata from response (properly this time, using omit from lodash)
• for simpler cases, also stopped using results in the effect, so that both results and metadata are not stored in state
• added defaultFilters prop so that isFiltering is still accurate in MainTable

In the future, we should also think about if we still need to store filters in additionalListeners. We might not need this any more.

[sjha4]

Noticed one remaining issue..

[jeremylenz]

@sjha4 It was because that page wasn't passing activeFilters to TableWrapper, so the check that reset it to page 1 wasn't happening. We should probably figure out a way to conditionally require that prop..

I also found another bug where if you have an active search (type = security), and try to advance the page, it would go into an infinite loop. I think that was because the pagination override logic was clearing paginationChangePending even when the page wasn't being updated. Take it for another spin and see if it's better..

[Andrewgdewar]

Phlegmless ACK!

[sjha4]

The issue I saw yesterday is reproducible still..I think it's only happening on filter detail pages so we may have something going on there..

Sorry I don't know how to record a single screen..

[jeremylenz]

@sjha4 That component also doesn't pass activeFilters. I'm going to make it a required prop; stand by...

[Andrewgdewar]

do we have to make it a required prop for tableWrapper? won't that just mean that a ton of tables will pass in an empty array

[jeremylenz]

Ended up not changing the required props. We probably should think about a solution though, as we now have 2 unenforced requirements:

1. Any filters must be passed as activeFilters and defaultFilters props
2. Filters must also be passed as additionalListeners

anyway, it's ready for review again :)

[jeremylenz]

Filters must also be passed as additionalListeners

I'm thinking maybe it's safer to have activeFilters always treated as if they are additionalListeners, and not require you to add them explicitly. thoughts? (Could be a follow-up PR.)

[sjha4]

Filters must also be passed as additionalListeners

I'm thinking maybe it's safer to have activeFilters always treated as if they are additionalListeners, and not require you to add them explicitly. thoughts? (Could be a follow-up PR.)

Like the idea to avoid duplicating putting filters in 2 props..
In the context of this PR, I think all issues are resolved.. 🥳 Ack 👍🏽