fix(RHINENG-9836) duplicate fetch on systems table #2245
Conversation
| ## autoRefresh | ||
|
|
||
| *boolean* | ||
|
|
||
| When `true`, the table is refreshed when `customFilters` are changed. | ||
|
|
||
| ## initialLoading | ||
|
|
||
| *boolean* | ||
|
|
||
| When `true`, the table is in loading state on mount until `entities.loaded` is set to `false` (and from that point, `loaded` is the only determinator.). Use when users can go back to already loaded table, this prop ensures that there will be no change from `loaded` > `loading` > `loaded`. | ||
|
|
||
| ## ignoreRefresh | ||
|
|
||
| *boolean = true* | ||
|
|
||
| On the initial mount and when items/sortBy are changed, the inventoryTable ignores `onRefresh` prop. By setting the prop to false, you can control this behavior. | ||
|
|
There was a problem hiding this comment.
These parameters can be used by other applications that use InventoryTable. Have we checked if this is not a breaking change?
There was a problem hiding this comment.
As far as I remember when making the original changes, these props were either already not used or the changes removed the behaviour these props would enable and made them obsolete.
The behaviours these props enabled were mostly workarounds and unintuitive. For example the autoRefresh is kinda redundant, because we should always refresh the table when customFilters change, otherwise we have the contents and filters out of sync. This was most likely a "fix" for a bug that caused the table not to get updated automatically.
However, in any case we should test these changes with some/all apps and maybe do a quick GitHub search for these specifically.
There was a problem hiding this comment.
OK, I think I understand. Still, there are a bunch of applications that use these parameters. Which implies a chance that this PR will introduce bugs:
https://github.com/search?q=org%3ARedHatInsights+autoRefresh&type=code
https://github.com/search?q=org%3ARedHatInsights+initialLoading&type=code
https://github.com/search?q=org%3ARedHatInsights+ignoreRefresh&type=code
@johnsonm325, could we make sure this PR is tested with the apps that are listed in these searches?
johnsonm325
force-pushed
the
rhineng-9836
branch
2 times, most recently
from
3cdcaad to
21c8232
Compare
| ref={ref} | ||
| onRefreshData={onRefreshData} | ||
| loaded | ||
| hasAccess={true} |
There was a problem hiding this comment.
We need to look into why we now need to pass this.
There was a problem hiding this comment.
@bastilian It's because I've changed default hasAccess to false, which is the correct thing to do until you fetch privileges. It seems like a great security measure. Do you agree?
Inventory makes multiple, duplicate, request on page load. Load up inventory to then see that these request are sent up multiple times. - Issue is reproducible when you apply some filters. For example, apply any OS filter and in network tab search for `/api/inventory/v1/hosts` - you will see 2 duplicated requests for hosts and for tags - Issue is reproducible when you apply global filters - Issue is also accompanied by the problem of missing debounce - Issue is visible on Advisor -> Recommendation Details page even without applying any additional filters This PR fixes issues where too many refreshes were being called from too many locations. We add a debounced call to the refresh data function and pass that debounce down where needed. We also cleaned up react hook dependencies and consolidated them.
johnsonm325
force-pushed
the
rhineng-9836
branch
from
21c8232 to
5560990
Compare
|
Let's focus more on the main intention, fixing the duplicate requests issue. We can follow up with cleaning up props in a new PR. |
No description provided.