Reduce api requests#1121
Conversation
The main navigation component fetches information for the page it is loading. As far as I understand it, the idea is to prefetch data to have it ready on page load. This makes pages load slower. Furthermore, the page itself still has to fetch that information anyway, because it might be loaded by other means, meaning we are doing the same (potentially expensive) backend requests twice. Therefore, this patch removes all backend requests from the main navigation component.
Ensure the redux state for "table" is completely cleared when a page is loaded.
...which results in more lines of code, but also more readability.
When unmounting a page, dispatched actions will continue to run in the background. This could cause the wrong data being loaded into the table, because the dispatched actions from one page would complete later than the dispatched actions from another page. This is most obvious when switching from series to events and then quickly switching back to series. This patch adds an interrupting mechanism that should prevent the "loadIntoTable" action to be dispatched if the component was unmounted.
This should only be called if the filters were actually changed,
not every time the TableFilters component is loaded.
This could cause weird effects by sometimes loading the wrong data.
This patch should fix that.
Much like the main navigation component, the navigation bar component has no business fetching information from the backend that the component would fetch by itself anyway.
Fetching data for a page would sometimes use the sorting params from the previous page, as the "resource" variable had not yet updated in the tableSlice. This could lead to wrong requests which then had to be corrected by subsequent requests. So instead of relying on the "resource" variable in the tableSlice, the fetching functions now hardcode their respective resource.
When fetching events, we would then make subsequent backend calls and await on each of them before returning. This would cause the response time to be several seconds instead of milliseconds even for a low number of events. This moves the subsequent backend calls into the component that actually requires the information so it can make the calls itself without delaying the table loading.
Apparently we can't just reset the whole tableSlice as we store non-temporary information (sorting parameters for all tables) in there. Therefore, this patch changes the reset function so that only removes actual table contents.
The stats are not needed in the events component, and the stats component is perfectly capable of fetching its own information.
No need to set an interval for every published cell, when we could just make the call dependent on the row passed to it.
The events component repeatedly fetched stats before, so now that the stats component is responsibly for it it should do that as well.
Includes opencast#980, supersedes opencast#1083. Should achieve the same thing as opencast#1083, but without reverting to older dependency versions. Meaning clicking on a series name in the series table should bring up the events table with the series filter set. Including opencast#980 as it helps in achieving this.
They don't need to be dispatching all that. The components will do it themselves when necessary. Also only fetch filter information if we don't already have an applicable filter in our redux store to save on loading times.
Some components are trying to set filters before loading a new table. However, the react-router link will have loaded the new page before the filters are actually set, leading to the display of unfiltered data. This hopes to fix that by properly awaiting before navigating.
|
Use Run test server using develop.opencast.org as backend: Specify a different backend like stable.opencast.org: It may take a few seconds for the interface to spin up. |
|
This pull request is deployed at test.admin-interface.opencast.org/1121/2025-03-19_09-34-07/ . |
The table limit (how many table entries should be shown) should not be reset when swichting between tables apparently.
|
This pull request has conflicts ☹ |
|
This pull request has conflicts ☹ |
|
This pull request has conflicts ☹ |
|
I wanted to check the performance with this PR in our test environment, but unfortunately it looks like you need Opencast version 17 for this PR to work (we currently have version 16 installed). However, my local tests of this PR, connected to https://stable.opencast.org, with various filters, switches between tabs etc. were successful. I also wanted to compare the performance when selecting a series in the series tab (as I did on #940), but this wasn't possible as the series filter on https://stable.opencast.org doesn't work at all at the moment. |
ferishili
left a comment
ferishili
left a comment
There was a problem hiding this comment.
Thanks @Arnei
Apart from the single code-related issue/suggestion, I found a few functional issues as follows:
Sorting issues:
Event Table
- Date sorting does not work.
- Start Time sorting does not work.
- End Time sorting does not work.
- Presenters sorting does not work.
- Status (PublishCell) throws an error.
Series Table
- Organizer(s) sorting returns event 400 error!
Servers Table
- Job completed sorting returns 400 error!
HTML Content Issue:
A dev-tool error appears:
In HTML, <button> cannot be a descendant of <button>.
This occurs due to two closing </button> elements, specifically in:
https://github.com/opencast/opencast-admin-interface/blob/ffde25bcd82d8aefc485acbdcbb20fb0b9ef6293/src/components/shared/Table.tsx#L307
This happens when selecting the table items num from the dropdown!
Bulk Checkbox Selector Issue
When the Events page first loads, the bulk select checkboxes are missing.
You need to switch between "Series" and "Events" for them to appear.
Some of the issues mentioned above might not be directly relevant to this PR. However, I am highlighting them here because I encountered them while performing functional testing on this PR.
This patch fixes a bug that happened when switching pages in the events table. Events that had no publication would be displayed as having publications, if the event in its place on the previous page had publications. For example, if the first event on page 1 had publications, and then you would switch to page 2 where the first event did not have publications, the event would be displayed as having publications. This patch fixes that by firstly resetting publication information if the information changes. It also limits enriching to only happen if the publications have changed.
|
Thanks for the extensive testing.
That should be fixed by backend PR opencast/opencast#6501. While the PR is already merged, I suppose it has not quite ended up on develop.opencast.org yet.
Is hopefully addressed by latest commit d474710. If that is not the case, please described what exactly you did to provoke the error message. Please also describe if anything bad is happening in the ui.
Good catch! As far as I can tell at a glance, these seem to be backend issues not caused by this PR. I'll open tickets for them.
Also unrelated to this PR it seems, I will create a ticket for it.
|
|
This pull request has conflicts ☹ |
ferishili
left a comment
ferishili
left a comment
There was a problem hiding this comment.
In this case, I would say this PR is good to go!
P.S.: Apologies for my previous mistaken review!
3 participants
Reduce the overall number of api requests we send to the backend when loading a new page/table. In particular, we were sending the same requests twice or more which should not be necessary. This cuts down on the number of requests in an attempt to reduce overall loading times. The guiding principle was that each component should be responsible for fetching its own data.
Should fix #940.
How to test this
Can be tested as usual. Switch between tables and modify the tables (filters, change page, change columns). Also check the requests in the network tab of your browser. (Note that if you test in development mode, due to React StrictMode being enabled requests will still be send twice).