wip on endless scrolling pagination #2221

antgamdia · 2020-12-17T20:41:00Z

Description of the change

This (endless) PR adds endless scrolling in the Catalog view as a result of the analysis and discussions of the design document.

Currently, in this PR we perform a server-side filtering for the most expensive calls: 1) filter by text; 2) filter by repository.
Moreover, the available categories are retrieved from the server-side (instead of getting unique names each time on client-side).

I decided to go this way because it results in faster response times from a UX perspective. I mean: to the best of my knowledge, there are no large repos adding meta information such as the categories. The typical use case would be people using bitnami catalog or other hand-crafted repo; in both cases, the amount of charts is still handleable (78 charts, 0.6 seconds)
Assuming this hypothesis, a server-side filtering here would increase the (perceived) response time: click filter, blank page with a spinner, render charts (instead of instantaneously filtering them)

Some notes about the PR

The scroll event triggers a bunch of updates -but- due to network issues or other causes, race conditions may occur. A cancelation policy should be implemented, but I'm still not able to figure out a good one; help wanted here.
Tests aren't updated yet since I'm waiting to get your first impressions on the overall implementation.
Discuss if we really want to move the category filtering to the backend

Current status

This PR is to be split into smaller ones. As I'm creating the new ones, I'm removing the corresponding changes here.

- Minor change in assetsvc (Use ChartQuery in getChartCategories #2244)
- Minor unhandled rejection (Fix unhandled rejection #2245)
- Add logic to retrieve the list of categories via backend (Retrieve chart categories using the backend #2246)
-Add logic to move text search to backend + force fetch when the namespace is changed (required for the implemented logic) (Move text filter to the backend #2249)
- Get allRepos from the state instead of iterating all charts to get them (Use repos from state in catalog #2251)
- Change the endpoint to the paginated ones, but without actually using it (Add pagination params #2252)
- Implement the endless pagination indeed! (Infinite scrolling pagination #2264)

Implementation details

The main changes are in these files:

Catalog.tsx
- It still has the filters for, each time the component is being loaded, applying some filters in client-side for chart
  - Type: either chart or operator
  - Category (list populated by server-side call)
- The operators part remains unchanged, all operators are being rendered without pagination.
- The scroll down event is detected using the IntersectionAPI of the browser. This is a widely supported browser API, but could eventually fail in old browsers, but I don't think it''s a concerning issue.
- The existing fetchCharts function has been replaced with fetchChartsWithPagination. Some attributes like query, page and size have been introduced.
- This function is called in different points: 1) during the component is mounted or there is a change in the namespace, 2) when the user scrolls down, 3), when the selected repository changes, 4) when there is an error an a manual load button appears,
actions/charts.ts
- Some new actions have been added for managing the paginated fetching process: reachEnd, receiveChartCategories, receiveChartsSearch, requestChartsCategories, requestChartsSearch, requestChartsVersions, resetChartsSearch, resetPaginaton
reducers/charts.ts
- Each of the aforementioned actions has its corresponding reducer function to update the state. It not only takes into account the action, but the status (idleStatus, errorStatus, loadingStatus, finishedStatus). It would be really useful to have a state diagram here, one can get lost very likely...

The remaining changes are less important (adding server URL with query params in url.ts, modifying the search box in SearchFilter.tsx to pass the current value and adding properties in types)

Benefits

Large repositories (>2000) won't take more than 5 seconds in being rendered as each element will be created while the user scrolls down.

Possible drawbacks

Since the scroll event is captured regardless of the current filter, we are triggering the loading even if it is not required.
For instance, in this example, we are retrieving the whole set of charts but displaying only the operators. This also happens in the filtering by category.
This is not that easy, because when rendering the view the first time, it will update the state to be "fetching charts" instead of "finish". Client-side filters do not modify the state.

Applicable issues

fixes Pagination for Catalog page #1010

Additional information

Design discussion at via.vmw.com/2020-kubeapps-ui-proposal.

Current status:

absoludity · 2021-01-08T00:47:04Z

This PR is waiting to be rebased I assume?

antgamdia · 2021-01-11T07:42:26Z

This PR is waiting to be rebased I assume?

You're right. I'll merge upstream changes, resolve conflicts and update the PR description adding more information.
Note, I still have to update the frontend tests.

absoludity

This (endless) PR adds endless scro

Haha - yes, it can feel that way sometimse!

absoludity · 2021-01-12T01:42:59Z

dashboard/src/actions/charts.ts

+  query: string,
+  page: number,
+  size: number,
+  nextPage: number,


Just for my own understanding, there's 3 changes here:

repo -> repos - the query now enables charts from multiple repos

addition of query - not yet sure why

Addition of pagination vars - why page and nextPage?

Right,
i) repos is because we can select multiple repos.;
ii) query is for searching charts using free text, when ""it doesn't perform any search;
iii) page and size is just for the usual pagination. nextPage is a workaround for attempting to solve the race condition when requesting

absoludity · 2021-01-12T01:54:11Z

dashboard/src/actions/charts.ts

-      const charts = await Chart.fetchCharts(cluster, namespace, repo);
-      if (charts) {
-        dispatch(receiveCharts(charts));
+      if (page === nextPage && !(query === "" && size === 0)) {


A comment here would be helpful... it's not immediately clear what this is doing. It looks like: "we only request charts if there's either a query or the size is non-zero" (still not clear what nextpage is for, but I'll find out below).

Yes, totally agree, it isn't clear at all. It has to do with the workaround for skipping useless requests. We should get rid of the nextPage and solve the cancelation of requests in another place.

absoludity · 2021-01-12T03:49:00Z

dashboard/src/actions/charts.ts

+  cluster: string,
+  namespace: string,
+  repos: string,
+  query: string,


It's not clear why we need a separate function here if the previous one includes a query option too? Only the actions look different?

It was just a reminiscence of old code. This duplicated function can be safely removed. Thanks for the observation!

absoludity · 2021-01-12T04:00:04Z

dashboard/src/actions/charts.ts

+    try {
+      const categories = await Chart.fetchChartCategories(cluster, namespace, repos);
+      if (categories) {
+        dispatch(receiveChartCategories(categories));


I always find it odd that we don't dispatch a receive when we receive zero categories (ie []), but I think it's what other actions do.

Yes, I just tried to replicate the behavior of other actions in this case

absoludity · 2021-01-12T04:00:54Z

dashboard/src/actions/repos.ts

+      dispatch(receiveReposSecrets(repoSecrets));
+    } catch (e) {
+      dispatch(errorRepos(e, "fetch"));
+    }


Previously unhandled error? Nice.

Yes, I faced this unhandled rejection when developing and just solved it here since it is a very minor fix

absoludity · 2021-01-12T04:10:23Z

dashboard/src/components/Catalog/Catalog.tsx

+      dispatch(actions.charts.resetChartsSearch());
+      fetchChartsWithPagination(cluster, namespace, currentRepo, "", 1, size, nextPage); // get the first charts
+      getCSVs(cluster, namespace);
+      dispatch(actions.charts.resetPaginaton()); // start from page=1 again


Why? Won't this mean the next page of results requested will be the first page again? (guessing not, but if you could expand the comment to explain why that'd be helpful.

Right now I'm not able to recall why the resetPaginaton() was useful. I remember I was getting page=2 results even if I changed the filters, but I'm not able to reproduce it right now. Commenting this line, but not deleting it just in case.

absoludity · 2021-01-12T04:31:00Z

dashboard/src/components/Catalog/Catalog.tsx

+    (query: string) => {
+      setCurrentSearchQuery(trimStart(query));
+      if (currentSearchQuery.length) {
+        if (currentSearchQuery.length < 5 || currentSearchQuery.endsWith(" ")) {


// If the user is still typing a query, throttle requests to a sensible frequency,
// otherwise debounce the requests so we only get a single request for the last event.

though, it's still not clear why we don't just use debounce regardless of whether the last character was a space? If debounce had 400ms and we used it exclusively, we'd just get a request every time the user paused (for more than 400ms). Why do we want the throttled one which will continually request when there are less that 5 letters?

I followed this post: https://www.peterbe.com/plog/how-to-throttle-and-debounce-an-autocomplete-input-in-react
Previously, I did a PoC of autocompletion in the search box and that's why I followed this twofold approximation.
Perhaps just debouncing may work.

absoludity · 2021-01-12T04:31:36Z

dashboard/src/components/Catalog/Catalog.tsx


  const filteredCharts = charts
    .filter(
      () => filters[filterNames.TYPE].length === 0 || filters[filterNames.TYPE].includes("Charts"),
    )
    .filter(() => filters[filterNames.OPERATOR_PROVIDER].length === 0)
-    .filter(c => new RegExp(escapeRegExp(searchFilter), "i").test(c.id))
+    // .filter(c => new RegExp(escapeRegExp(searchFilter), "i").test(c.id))


Guessing you meant to remove this.

absoludity · 2021-01-12T04:32:14Z

dashboard/src/components/Catalog/Catalog.tsx

+      () => filters[filterNames.TYPE].length === 0 || filters[filterNames.TYPE].includes("Charts"),
+    )
+    .filter(() => filters[filterNames.OPERATOR_PROVIDER].length === 0)
+    // .filter(c => new RegExp(escapeRegExp(search.query), "i").test(c.id))


Same here (remove?)

absoludity · 2021-01-12T04:35:41Z

dashboard/src/components/Catalog/CatalogItems.tsx

+  csvs,
+  cluster,
+  namespace,
+  hasFinished,


hasFinished isn't really descriptive. I assume this means hasFinishedFetching?

antgamdia · 2021-01-12T08:12:05Z

Thank you for your comments despite it is still a draft PR. I wanted to polish up a couple of things (such as solving some issues when merging and adding more comments) and add some implementation details in the PR comment.
As you have already noticed, there is some odd stuff. It was more complicated than I expected, and, in fact, I wasn't able to effectively solve the issue of canceling requests.
For instance, the nextPage is just an attempt to ensure each page is being requested (despite duplicate requests may and will occur). Nevertheless, I'd like to discuss it a little bit more with you folks.

antgamdia · 2021-01-13T11:45:19Z

As per the discussion with @andresmgot, this PR should be split into many smaller PRs, namely:

- Minor change in assetsvc (Use ChartQuery in getChartCategories #2244)
- Minor unhandled rejection (Fix unhandled rejection #2245)
- Add logic to retrieve the list of categories via backend (Retrieve chart categories using the backend #2246)
-Add logic to move text search to backend + force fetch when the namespace is changed (required for the implemented logic) (Move text filter to the backend #2249)
- Get allRepos from the state instead of iterating all charts to get them (Use repos from state in catalog #2251)
- Change the endpoint to the paginated ones, but without actually using it (Add pagination params #2252)
- Implement the endless pagination indeed! (Infinite scrolling pagination #2264)
Once we have everything merged, I will send another PR with the problematic changes so that the scope of them is reduced.

The main problem is when making multiple requests: we should track each one and determine if it should be accepted (add chunks), ignored (the data shouldn't be added) or retried (if it failed)

This reverts commit 77b3b6e.

This reverts commit 84ddf90.

antgamdia · 2021-01-15T13:31:30Z

For the record, minor UI bug discovered when provider's name includes a comma. Users won't be able to deselect it, resulting in something like:

antgamdia · 2021-01-18T15:02:53Z

With a proper useEffect and reducers usage (and a map to track the requests to avoid dups), now scrolling-triggered requests are working as expected!
Further efforts still needed on handling different changes (selected repos, selected namespace, update text search, change view, refresh page, etc).

Current status:

Note: no PR open yet since I want to address other PR's comments (and learn from them :) ) first.

antgamdia · 2021-01-20T15:22:38Z

Closing this PR since it has been moved to #2264.

antgamdia added 5 commits December 17, 2020 21:32

wip on endless scrolling pagination

fdabd7f

Merge branch 'master' into 1010-UIpagination

4ac882b

Merge branch 'master' into 1010-UIpagination

bc0a440

wip on endless scrolling. improve filtering logic

b89033c

Fix unhandled rejection

84ddf90

antgamdia added 5 commits January 11, 2021 12:37

partial cherry pick cherry pick 6ca62d6

e1dfee7

Use ParsedQs in catalog.tsx

32ce519

Merge branch 'master' into 1010-UIpagination

bae8164

Fix wrong merged files

d0f3624

Add missing padding in OperatorList view

1405de5

absoludity reviewed Jan 12, 2021

View reviewed changes

Merge branch 'master' into 1010-UIpagination

fbf741c

antgamdia added 4 commits January 12, 2021 10:41

Use ChartQuery in getChartCategories

77b3b6e

Merge branch 'master' into 1010-UIpagination

e74ece7

Address minor PR comments

a6fbff4

Merge branch 'master' into 1010-UIpagination

4df984b

Merge branch 'master' into 1010-UIpagination

a458aaf

antgamdia mentioned this pull request Jan 13, 2021

Use ChartQuery in getChartCategories #2244

Merged

Revert "Use ChartQuery in getChartCategories"

8f45716

This reverts commit 77b3b6e.

antgamdia mentioned this pull request Jan 13, 2021

Fix unhandled rejection #2245

Merged

antgamdia added 3 commits January 13, 2021 20:09

Revert "Fix unhandled rejection"

f4f02ae

This reverts commit 84ddf90.

Removed unused CatalogItemLoader

bb2b853

Uncomment non-paginated list charts url

4058700

antgamdia mentioned this pull request Jan 13, 2021

Retrieve chart categories using the backend #2246

Merged

Remove getCategories logic

11f5409

antgamdia mentioned this pull request Jan 14, 2021

Move text filter to the backend #2249

Merged

This was referenced Jan 16, 2021

Use repos from state in catalog #2251

Merged

Add pagination params #2252

Merged

antgamdia mentioned this pull request Jan 20, 2021

Infinite scrolling pagination #2264

Merged

6 tasks

antgamdia closed this Jan 20, 2021

antgamdia deleted the 1010-UIpagination branch January 25, 2021 07:34

antgamdia mentioned this pull request Jan 25, 2021

Change comma by pipe to concat filters in the catalog #2279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip on endless scrolling pagination #2221

wip on endless scrolling pagination #2221

antgamdia commented Dec 17, 2020 •

edited

Loading

absoludity commented Jan 8, 2021

antgamdia commented Jan 11, 2021

absoludity left a comment

absoludity Jan 12, 2021

antgamdia Jan 12, 2021

absoludity Jan 12, 2021

antgamdia Jan 12, 2021

absoludity Jan 12, 2021

antgamdia Jan 12, 2021

absoludity Jan 12, 2021

antgamdia Jan 12, 2021

absoludity Jan 12, 2021

antgamdia Jan 12, 2021

absoludity Jan 12, 2021

antgamdia Jan 12, 2021

absoludity Jan 12, 2021

antgamdia Jan 12, 2021

absoludity Jan 12, 2021

absoludity Jan 12, 2021

absoludity Jan 12, 2021

antgamdia commented Jan 12, 2021

antgamdia commented Jan 13, 2021 •

edited

Loading

antgamdia commented Jan 15, 2021

antgamdia commented Jan 18, 2021

antgamdia commented Jan 20, 2021

wip on endless scrolling pagination #2221

wip on endless scrolling pagination #2221

Conversation

antgamdia commented Dec 17, 2020 • edited Loading

Description of the change

Some notes about the PR

Current status

Implementation details

Benefits

Possible drawbacks

Applicable issues

Additional information

absoludity commented Jan 8, 2021

antgamdia commented Jan 11, 2021

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgamdia commented Jan 12, 2021

antgamdia commented Jan 13, 2021 • edited Loading

antgamdia commented Jan 15, 2021

antgamdia commented Jan 18, 2021

antgamdia commented Jan 20, 2021

antgamdia commented Dec 17, 2020 •

edited

Loading

antgamdia commented Jan 13, 2021 •

edited

Loading