Infinite scrolling pagination #2264

antgamdia · 2021-01-20T13:47:50Z

Description of the change

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

We already implemented the 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).

In this PR we support endless pagination, so once the user has reached the bottom of the page, new chunks will be requested.

Benefits

Users won't experience a long delay when rendering the whole set of charts, since the catalog will be retrieved little by little.

Possible drawbacks

Currently, there are issues when triggering many requests at a time. For instance, for replicating this failure scenario, rapidly click on a repository checkbox multiple times and scroll up and down. In the network tab, some requests don't make sense: repeated ones, race conditions. etc
Currently, we don't retry failed requests.

Applicable issues

fixes Pagination for Catalog page #1010

Additional information

Change initially introduced in #2221 but removed from there to keep it as simple as possible.

Specifically, this PR is being sent after having implemented the necessary code in other places for the pagination to work, 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)

Some implementation details:

First render:

page=1
hasRequestedFirstPage=false
hasLoadedFirstPage=false
isFetching=false
useEffect(initial)-> fetchCharts with page=1
isFetching=false
useEffect(isFetching changed to true) -> hasRequestedFirstPage=true
useEffect(hasRequestedFirstPage changed to true) -> hasLoadedFirstPage=true

Scrolling

user scrolls down and, because hasLoadedFirstPage is true -> page=page + 1
useEffect(page changed to)-> fetchCharts with page=2

andresmgot · 2021-01-20T14:44:50Z

It's usually better to review finished PRs but I am okay giving some early feedback

andresmgot

Some comments here, the major two things I would change is having the page in the Catalog state and use the total number of pages that the API should return to know if you are in the latest page or not.

andresmgot · 2021-01-20T14:49:20Z

dashboard/src/actions/charts.ts

-    } catch (e) {
-      dispatch(errorChart(new FetchError(e.message)));
+    const latestElement = Array.from(records.keys()).pop() || 1;
+    if (records.get(page) === false && page <= latestElement) {


I don't undestand the condition page <= latestElement, shouldn't be page bigger than the latest page in the records? (BTW, I would change latestElement for lastPage if that's what it means).

It is just an extra condition for my unfruitful attempts to avoid the problems of duplicating requests.
The rationale behind this is latestElement is being set in receiveCharts: here we set to false the next page that should be requested (otherwise, it should be undefined).

So, if is properly processes page 6, it will set 7: false. During the next request, (records.get(7) === false && 7 <= 7)

If something is wrong, the map could possibly include 7:false, 8:false , the latestElement would be 8 whereas page is still 7, and that's precisely what this change is trying to prevent.

I'm sure there exists a better solution for the root cause.

Regarding the renaming, it's not the lastPage, but the lastPendingPage, but I'm ok with whichever name.

andresmgot · 2021-01-20T14:52:38Z

dashboard/src/actions/charts.ts

+      dispatch(requestCharts(page, query));
+      try {
+        const charts = await Chart.fetchCharts(cluster, namespace, repos, page, size, query);
+        dispatch(receiveCharts(charts, charts.length === 0));


I don't think charts.length === 0 is a mechanism to know if the list has reached the end (it may be the result of asking for a wrong query).

In any case, I am pretty sure that the API endpoint returns the number of pages in the response (at least it was that way last time I checked it). You should compare that number with the current page instead.

Yes, I agree, I'm going to change this condition and leverage from the totalPages value already present in the API response. Totally, right!

andresmgot · 2021-01-20T14:56:51Z

dashboard/src/components/Catalog/Catalog.tsx

  useEffect(() => {
-    fetchCharts(cluster, namespace, reposFilter, page, size, searchFilter);
-  }, [fetchCharts, cluster, namespace, reposFilter, page, size, searchFilter]);
+    clusterUpdate.current = cluster;
+    namespaceUpdate.current = namespace;
+    reposFilterUpdate.current = reposFilter;
+    searchFilterUpdate.current = searchFilter;
+    resetRequestCharts();
+  }, [resetRequestCharts, cluster, namespace, reposFilter, searchFilter]);


I am not sure if calling resetRequestCharts is the correct approach but the useRefs are not needed for this as far as I can see.

If I understand the problem here, if you are in page 3 and then you click in a repo filter, you want to reset the page counter, right? (So you request the first page again). Or you are trying some performance improvement?

What I would do, to try to simplify this a bit (and potentially avoid the refs), would be to have the page in the component state (rather than in redux), and everytime there is a change in the filters, reset the page. I am not sure if that'd trigger two requests though (one for the page: 0 and old filters and one for page: 0 and the new filters or just the second).

I used the useRef beacuse of I explained in #2264 (comment).

Mmm, but, although we store page in the component state, we will still have it as one of the dependencies of the useEffect, so, regardless of how it is stored, it would cause a refresh whenever it is updated, wouldn't it?

I don't know, anyway I will give it a try for sure.

yes, but that's what we want, no? If we are at page: 2, repo: "" and we change it to repo: "bitnami and at the same time (this is what I don't know if it's possible) page: 0 we would do a single fetchChart with page: 0, repo: "bitnami". Isn't that correct?

andresmgot · 2021-01-20T14:58:26Z

dashboard/src/components/Catalog/Catalog.tsx

+  const debouncedFetchCharts = useCallback(
+    debounce(() => {
+      fetchCharts(cluster, namespace, reposFilter, page, size, records, searchFilter);
+    }, 500),


Again, I am not sure useCallback + debounce works as you expect, double check it if it's really doing anything.

As far as I could check, the denounce is working accordingly working with the specified time (1s in the gif):

Otherwhise, if we remove the debounce and the useCallback each request is being duplicated since we wouldn't store the returned values of the function. Here's an example:

Nice, thanks for double-checking this. You will need to add some tests for this though. Also, I think that the usage of debounce is legitimate but useCallback seems a hack for fixing the problem of not re-requesting the same page (in theory we should be taking the count of the pages we have already requested, and in fact we are, and use that to decide to trigger or not the function).

In any case, I am fine with this as long as you ensure it with a test.

andresmgot · 2021-01-20T15:02:03Z

dashboard/src/components/Catalog/Catalog.tsx

+                  // Enable scrolling just if every parameter is already updated
+                  clusterUpdate.current === cluster &&
+                  namespaceUpdate.current === namespace &&
+                  reposFilterUpdate.current === reposFilter &&
+                  searchFilterUpdate.current === searchFilter


I don't understand these conditions, I believe is trying to avoid duplicated calls but can you elaborate?

Yes, I'd want to explain it a little bit more as it is just another attempt to avoid dups:

Whenever a change occurs and it is detected by the useEffect, I (try to) prevent these values to be actually updated before triggering the reset. It is being done by means of the useRef. The good point here is that mutating the .current won't cause a re-render.

If we remove these conditions, duplicated requests would appear.

that's fine then but as above, you'd need to ensure this with a test. First, comment this code, and write a test ensure it's failing (there are more than one request for the same params), then enable the functional code and the test should pass.

andresmgot · 2021-01-20T15:05:41Z

dashboard/src/reducers/charts.ts

    case getType(actions.charts.requestChartsCategories):
      return { ...state, isFetching: true };
    case getType(actions.charts.receiveCharts):
+      state.records.set(state.page, true);
+      state.records.set(state.page + 1, false);


do you need this line? Isn't the default false?

I think so; when there is no key, the map.get(xxx) will return undefined instead of false (and this behavior is being used in #2264 (comment))

andresmgot · 2021-01-20T15:06:36Z

dashboard/src/reducers/charts.ts

-        items: action.payload,
+        hasFinished: action?.meta,
+        items: uniqBy([...state.items, ...action.payload], "id"), // TODO(agamez): handle undesired requests to avoid this workaround
+        page: action?.meta ? state.page : state.page + 1, // if action.meta==true, it's the last chunk


I would include the page in the payload

andresmgot · 2021-01-20T15:20:28Z

dashboard/src/reducers/charts.ts

      return {
        ...state,
        isFetching: false,
-        items: action.payload,
+        hasFinished: action?.meta,
+        items: uniqBy([...state.items, ...action.payload], "id"), // TODO(agamez): handle undesired requests to avoid this workaround


yep, +1 to the TODO, we should get rid of those duplicated requests

antgamdia · 2021-01-20T23:40:54Z

having the page in the Catalog state and use the total number of pages that the API should return to know if you are in the latest page or not.

Thank you, as usual, for your comments :)

Regarding the page in the Catalog state, I'm not completely sure about that. I'm using the state's page here to (try to) avoid duplicates. See what you think.
https://github.com/kubeapps/kubeapps/blob/1f22d91c57268f6e5d2bbc5b770bad57f32ca942/dashboard/src/reducers/charts.ts#L69

The second point has been addressed, a much better approach to detect the end of the pagination!

andresmgot

Thanks @antgamdia this is looking better! As a summary, I have two major comments:

There are many tests missing to ensure the behavior you are coding.
The decision to fetch or no charts should be made in the Catalog. Thanks to that, you may be able to remove (or not) the pages from the state.

Let me know what you think or if you are stuck testing something.

andresmgot · 2021-01-21T08:57:08Z

dashboard/src/actions/charts.test.tsx

+      {
+        type: getType(actions.charts.receiveCharts),
+        payload: { items: response, page: 1 },
+        meta: false,


I think you don't need the meta field now.

andresmgot · 2021-01-21T09:06:15Z

dashboard/src/actions/charts.ts

+    const lastPendingPage = Array.from(records.keys()).pop() || 1;
+    if (records.get(page) === false && page <= lastPendingPage) {


Following the conversation at #2264 (comment)

As a general comment, code should not be added "hoping" to behave somehow, it has to be proven to behave that way with unit tests. I don't see any unit tests enforcing the behavior of this so I'd assume this is not needed. Code that is not "protected" by unit tests is damned to be broken in the future when we don't remember what's doing and we decide to clean it up.

Regarding this issue in particular, I don't think is the duty of the function fetchCharts to decide if it should fetchCharts or not based on the state. It's the duty of the caller, in this case the Catalog component, to not request twice a page (and it's easier to add safeguards and tests there).

andresmgot · 2021-01-21T09:07:42Z

dashboard/src/actions/charts.ts

+      dispatch(requestCharts(page, query));
+      try {
+        const response = await Chart.fetchCharts(cluster, namespace, repos, page, size, query);
+        dispatch(receiveCharts({ items: response.data, page }, response?.meta?.totalPages <= page));


I would also add the totalPages to the payload unless using the meta parameter as you are doing gives some benefit, that way, if in the future you need to add more info, you don't need to guess if it's in one parameter or the other.

andresmgot · 2021-01-21T09:13:38Z

dashboard/src/components/Catalog/Catalog.tsx

  useEffect(() => {
-    fetchCharts(cluster, namespace, reposFilter, page, size, searchFilter);
-  }, [fetchCharts, cluster, namespace, reposFilter, page, size, searchFilter]);
+    clusterUpdate.current = cluster;
+    namespaceUpdate.current = namespace;
+    reposFilterUpdate.current = reposFilter;
+    searchFilterUpdate.current = searchFilter;
+    resetRequestCharts();
+  }, [resetRequestCharts, cluster, namespace, reposFilter, searchFilter]);


yes, but that's what we want, no? If we are at page: 2, repo: "" and we change it to repo: "bitnami and at the same time (this is what I don't know if it's possible) page: 0 we would do a single fetchChart with page: 0, repo: "bitnami". Isn't that correct?

andresmgot · 2021-01-21T09:18:02Z

dashboard/src/components/Catalog/Catalog.tsx

+  const debouncedFetchCharts = useCallback(
+    debounce(() => {
+      fetchCharts(cluster, namespace, reposFilter, page, size, records, searchFilter);
+    }, 500),


Nice, thanks for double-checking this. You will need to add some tests for this though. Also, I think that the usage of debounce is legitimate but useCallback seems a hack for fixing the problem of not re-requesting the same page (in theory we should be taking the count of the pages we have already requested, and in fact we are, and use that to decide to trigger or not the function).

In any case, I am fine with this as long as you ensure it with a test.

andresmgot · 2021-01-21T09:21:46Z

dashboard/src/components/Catalog/Catalog.tsx

+                  // Enable scrolling just if every parameter is already updated
+                  clusterUpdate.current === cluster &&
+                  namespaceUpdate.current === namespace &&
+                  reposFilterUpdate.current === reposFilter &&
+                  searchFilterUpdate.current === searchFilter


that's fine then but as above, you'd need to ensure this with a test. First, comment this code, and write a test ensure it's failing (there are more than one request for the same params), then enable the functional code and the test should pass.

andresmgot · 2021-01-21T09:28:30Z

dashboard/src/reducers/charts.ts

+      // only update the records if the received page matches the requested one
+      const areMatchingPages = action.payload.page === state.page;
+      if (areMatchingPages) {
+        state.records.set(state.page, true); // mark the current page as successfully retrieved
+        state.records.set(state.page + 1, false); // mark the next page as pending (not undefined)
+      }
      return {
        ...state,
        isFetching: false,
-        items: action.payload,
+        hasFinished: action?.meta,
+        items: areMatchingPages
+          ? uniqBy([...state.items, ...action.payload.items], "id")
+          : state.items, // if pages don't match, ignore payload to avoid duplicates
+        page: action?.meta ? action.payload.page : action.payload.page + 1, // if action.meta==true, it's the last chunk
+        records: state.records,


I think we can simplify all this if we store items as a map. If we store:

items[action.payload.page] = action.payload.items

You will get unique items per page and, I think, you won't need the records since you already have a map.

I'm ok with this simplification general aim, but, don't you think it will increase complexity in the view? I mean, the operations would be pretty the same, but, I'm unsure of moving them to the component.

andresmgot · 2021-01-21T09:31:16Z

dashboard/src/reducers/charts.test.ts

+      records: new Map<number, boolean>()
+        .set(1, true)
+        .set(2, true)
+        .set(3, false),
    });
  });



I'd add a couple more of tests, one to check that receiving two pages of charts includes all the charts as items and another checking that if you add the same page twice the items are not duplicated.

andresmgot · 2021-01-21T09:32:16Z

dashboard/src/shared/types.ts

@@ -95,6 +104,7 @@ export interface IChartAttributes {

 export interface IChartState {
  isFetching: boolean;
+  hasFinished: boolean;


nitpipck: hasFinished could mean anything, a better name would be finishedFetching

antgamdia · 2021-01-21T11:34:27Z

The decision to fetch or no charts should be made in the Catalog. Thanks to that, you may be able to remove (or not) the pages from the state.

Again, thank you for your comments!

Just for the record, as you suggested in 1-1 conversations, we could do something like:

const [pagesRequested, setPagesRequested] = useState([] as number[]);
if (
    entry.isIntersecting &&
    !isFilteringOnlyOperators && 
    !hasFinished &&
    !pagesRequested.include(page)
  ) {
      setPagesRequested(pagesRequested.concat(page));
      debouncedFetchCharts();
  }

- hasFinished is substituted by totalPages and it is passed as part of the payload of requestCharts - hasFinished is renamed to hasFinishedFetching: false, in the chart state - delete unused query param at the requestCharts action - edit tests to match the aforementioned non-functional changes

- If a filter was not present after a change (i.e., a different ns with different repos) the "catalog is empty" msg appeared

antgamdia · 2021-01-22T00:15:23Z

As I mentioned in the (offline) discussion, I have not been able to move the page and its corresponding logic to the component. Besides my unfruitful attempts to do so, multiple requests are being generated.
Currently, in this PR this problem is addressed with a hacky combination of useRef, useCallback and debounce...

This situation also prevented me to write some test cases, I will ping you to discuss a little bit more on how to address them.
As far as I can remember, the current missing tests are:

Check that resetRequestCharts is invoked after a change is detected in cluster, namespace, reposFilter, searchFilter.
Check the debounce is really giving a rate of 2req/second.
Test the intersection API to simulate a scrolling event.

Apart from these three major (at least for me) tests, the PR will be finished.

andresmgot · 2021-01-22T10:43:00Z

dashboard/src/actions/charts.test.tsx

@@ -62,33 +81,6 @@ describe("fetchCharts", () => {
    );
  });

-  it("fetches charts with query", async () => {


why have you deleted this test? (even if the query is not sent in the action it's useful to ensure it's being used)

The action no longer has the "query" as part of the payload. Why would we have to still have this test for a non-existent parameter?

export const requestCharts = createAction("REQUEST_CHARTS", resolve => { return (page?: number) => resolve(page); });

Instead, they have been replaced by others testing the page parameter; namely:

fetches charts from a repo (first page) fetches charts from a repo (middle page) fetches charts from a repo (last page) fetches charts from a repo (already processed page) fetches charts from a repo (not-yet-requested page) fetches charts from a repo (already-requested page)

basically to ensure that the URL contains the query parameter

andresmgot · 2021-01-22T10:50:13Z

dashboard/src/reducers/charts.ts

@@ -70,14 +71,15 @@ const chartsReducer = (
        state.records.set(state.page, true); // mark the current page as successfully retrieved
        state.records.set(state.page + 1, false); // mark the next page as pending (not undefined)
      }
+      const isLastPage = action.payload.page >= action.payload.totalPages;


shouldn't this be page === totalPages? (not sure why the >=, if that's possible.)

Yes, in theory, we reach the end when the page===totalPages. It was just to cover the edge case in which two receiveCharts arrive at the same time and, "lastPage" and "lastPage+1" are both marked to be fetched. So I directly consider lastPage+n as the same case.

However, it should occur and, if it does, we can just ignore the subsequent lastPage+n requests.

…to pagination

andresmgot · 2021-01-22T10:57:37Z

dashboard/src/components/Catalog/Catalog.tsx

+                        <div className="endPageMessage">
+                          <LoadingWrapper loaded={false} />
+                          <span>
+                            {!charts.length && !csvs.length
+                              ? "Loading catalog..."
+                              : "Scroll down to discover more applications"}
+                          </span>
+                        </div>
+                      )}


two minor things here:

I would use the small loading spinner, I think there is a property for that

Would remove the messages (including "no remaining applications"), it's clear that it's loading when the loading wrapper is shown and it's weird when using filters. For example if I search "wordpress" I see the message "scroll down..." but there is no need to scroll down, is just a matter of letting it load.

I'll test the small spinner out, I remembered to have seen this prop and will add some screenshots for us to compare.

I'm not sure about that, searching for WordPress perhaps results in a few charts (and "no remaining applications" won't be displayed if searching). However, search results can possibly be paginated, so these messages do make sense for me. Again, I will change it and will post here some gifs for us to decide.

Here is a comparison of the spinners; perhaps I'll go for the medium-sized one.

andresmgot · 2021-01-22T11:00:15Z

Another usability issue, I see that operators are loaded first and shown for a few seconds, before getting the first page:

We should show only the loading wrapper before we get the first page.

andresmgot · 2021-01-22T11:04:30Z

Sorry, I am adding comments as I see them (I am checking the code locally).

I am also getting repeated requests, this is the load of the second page:

Is that expected?

Also, I think we should use smaller page sizes, 50 rather of 100 would be better IMO

antgamdia · 2021-01-22T11:36:26Z

We should show only the loading wrapper before we get the first page.

I don't get your point here, this is how it was working so far. What if only operators, and not charts, are present?
For doing so, we should have a kinda hasStartedFetching or similar check to be sure that we only are rendering them (charts/ops) if: 1) we already have charts OR 2) a chart retrieving process has initiated but there are no results.
Perhaps we can address it in another PR (we also have the issue when entering filters with ",", I was wondering about creating an issue with all this future work.

antgamdia · 2021-01-22T11:40:34Z

Sorry, I am adding comments as I see them (I am checking the code locally).

Not a problem at all! And thank you for checking manually the code :)

Also, I think we should use smaller page sizes, 50 rather of 100 would be better IMO

Yes, it was something I also wanted to discuss. For this PR, I can hardcoded it to 50, that's great. However, perhaps we should move it to the Config.ts so that it becomes more easily to be changed.

Pagination pair progr

The previous fix didn't completely solve the issue. Now charts and operators are rendered at the same time during the page 1 invocation. Added corresponding tests.

antgamdia · 2021-01-22T21:00:37Z

After a bunch of test cases and minor bug fixes after addressing the previous comments and code suggestions, there is a stable version. There are no duplicated requests and the overall UX should be also ok.
Some test cases are still pending; honestly, I can't figure out how to test in-depth how the pagination currently works (notes below) and how to better test the scrolling.

Given that the end of the iteration is coming, I would suggest prioritizing the essential pending aspects you may consider to this PR to be merged. And, next, creating an issue for documenting the further actions for further iterations.
For instance: i) search text box is not always in sync with the ?Search param; ii) I'd like to have a configurable size rather than hardcoding it; iii) there is an issue when filter values include a comma.

See what you think :)

First render:

page=1
hasRequestedFirstPage=false
hasLoadedFirstPage=false
isFetching=false
useEffect(initial)-> fetchCharts with page=1
isFetching=false
useEffect(isFetching changed to true) -> hasRequestedFirstPage=true
useEffect(hasRequestedFirstPage changed to true) -> hasLoadedFirstPage=true

Scrolling

user scrolls down and, because hasLoadedFirstPage is true -> page=page + 1
useEffect(page changed to)-> fetchCharts with page=2

andresmgot

Thanks! The functional code looks good to me (caveat below), I just have some minor comments regarding the tests.

iii) there is an issue when filter values include a comma.

I'm able to reproduce that bug and it's quite important. Can you open another PR to fix it before doing a release? It should be easy to fix (just ignore commas for the search filter).

andresmgot · 2021-01-25T09:36:14Z

dashboard/src/actions/charts.test.tsx

@@ -62,33 +81,6 @@ describe("fetchCharts", () => {
    );
  });

-  it("fetches charts with query", async () => {


basically to ensure that the URL contains the query parameter

andresmgot · 2021-01-25T09:37:36Z

dashboard/src/actions/charts.ts

+          items: response.data,
+          page,
+          totalPages: response.meta.totalPages,
+        } as IReceiveChartsActionPayload),


AFAIk you don't need the as IReceiveChartsActionPayload if the object you are passing contains all the fields.

Yes, this is not necessary at all, but I just wanted to type the object so that we can get transpiler errors when changing its props in the future. However, I'm ok removing it.

andresmgot · 2021-01-25T09:38:06Z

dashboard/src/components/Catalog/Catalog.test.tsx

+  hasFinishedFetching: false,
  selected: {} as IChartState["selected"],
  deployed: {} as IChartState["deployed"],
  items: [],
  categories: [],
  updatesInfo: {},
  page: 1,
-  size: 0,
+  size: 20,
+  records: new Map<number, boolean>().set(1, false),


you can remove hasFinishedFetching and records from here.

Yes, records was unnoticedly left there. But, hasFinishedFetching is still part of the IChartState, so it should remain, shouldn't it?
Edit: ah, you perhaps meant page, since it is now in the catalog state.

andresmgot · 2021-01-25T09:44:23Z

dashboard/src/components/Catalog/Catalog.test.tsx

+    spyOnUseState = jest
+      .spyOn(React, "useState")
+      //  @ts-ignore
+      .mockImplementation((init: any) => [init, setState]);
+    spyOnUseEffect = jest
+      .spyOn(React, "useEffect")
+      //  @ts-ignore
+      .mockImplementation((f, n) => f(mockUseEffect(n)));


you should not mock useState useEffect or any of the internals of the component (among other reasons, because you are changing the behavior of the component). The only thing you are interested here is that fetchCharts is called once, with the proper page

andresmgot · 2021-01-25T09:46:49Z

dashboard/src/components/Catalog/Catalog.test.tsx

+    expect(resetRequestCharts).toHaveBeenNthCalledWith(1);
+  });
+
+  it("sets state params when fetching charts", () => {


not sure what this test is ensuring, is it the same than the previous one? In any case, the same comment applies, you should not test/ensure the component internals but the outputs

andresmgot · 2021-01-25T09:48:02Z

dashboard/src/components/Catalog/Catalog.test.tsx

+    expect(resetRequestCharts).toHaveBeenCalledWith();
+  });
+
+  it("sets state params when charts have been fetched", () => {


same, here you can test for example that the items are translated to CatalogItems

andresmgot · 2021-01-25T09:51:52Z

dashboard/src/components/Catalog/Catalog.test.tsx

+    expect(resetRequestCharts).toHaveBeenNthCalledWith(1);
+  });
+});
+


I would add a test that checks that the page is set to 1 and that the resetCharts function is called when one of the filters change (if changing the page is not easy, at least check that the reset method is called).

this test is still not there, no?

Ups!! I totally forgot and missed this comment, sorry!

andresmgot · 2021-01-25T10:04:01Z

dashboard/src/reducers/charts.test.ts

+      isFetching: false,
+      hasFinishedFetching: false,
+      items: [chartItem],
+      // page: 3,


remove comment

andresmgot · 2021-01-25T10:04:20Z

dashboard/src/reducers/charts.test.ts

+      isFetching: false,
+      hasFinishedFetching: false,
+      items: [chartItem],
+      // page: 3,


andresmgot · 2021-01-25T10:56:00Z

dashboard/src/components/Catalog/Catalog.test.tsx

+    expect(resetRequestCharts).toHaveBeenNthCalledWith(1);
+  });
+});
+


I wanted to check if it's possible to mock the observer to test that at least we are changing the page. I managed to do so:

it("changes page", () => { const setState = jest.fn(); const setPage = jest.fn(); const charts = { ...defaultChartState, hasFinishedFetching: false, isFetching: false, items: [], } as any; spyOnUseState = jest .spyOn(React, "useState") // @ts-ignore .mockImplementation((init: any) => { if (init === false) { // Mocking the result of hasLoadedFirstPage to simulate that is already loaded return [true, setState]; } if (init === 1) { // Mocking the result of setPage to ensure it's called return [1, setPage]; } return [init, setState]; }); // Mock intersection observer const observe = jest.fn(); const unobserve = jest.fn(); window.IntersectionObserver = jest.fn(callback => { (callback as (e: any) => void)([{ isIntersecting: true }]); return { observe, unobserve } as any; }); mountWrapper(defaultStore, <Catalog {...populatedProps} charts={charts} />); expect(setPage).toHaveBeenCalledWith(2); });

Ideally we should check that fetchCharts is called with the second page, rather than mocking setPage but if we don't mock setPage, it enters an infinite loop increasing the page and if we mock it, the related useEffect is not called.

BTW, you may need to also mock window.IntersectionObserverEntry, I commented that part of the code when trying this

FTR:
The condition was: if ("IntersectionObserver" in window && "IntersectionObserverEntry" in window && "isIntersecting" in window.IntersectionObserverEntry.prototype)

Since mocking window.IntersectionObserverEntry.prototype is causing problems, we decided to get rid of this condition.
In the future, if we decide to implement a fallback mechanism (fetch all pages instead of one-by-one), we can add it back.

…ion test case.

andresmgot

Thanks for the changes, this is almost done :) Accepting it already once you add that test I mention

andresmgot · 2021-01-26T14:50:06Z

dashboard/src/components/Catalog/Catalog.test.tsx

-    expect(mockUseEffect).toHaveBeenNthCalledWith(3, [false, false]); // [hasRequestedFirstPage, isFetching])
-    expect(fetchCharts).toHaveBeenNthCalledWith(1, "default-cluster", "kubeapps", "", 1, 20, ""); // [hasRequestedFirstPage, isFetching])
-    expect(mockUseEffect).toHaveBeenCalledTimes(7);
+    expect(wrapper.find("CatalogItems>CatalogItem>ChartCatalogItem").length).toBe(0);


you can just do find(ChartCatalogItem), no?

andresmgot · 2021-01-26T14:56:26Z

dashboard/src/components/Catalog/Catalog.test.tsx

+    expect(resetRequestCharts).toHaveBeenNthCalledWith(1);
+  });
+});
+


this test is still not there, no?

absoludity · 2021-01-27T08:29:01Z

Don't wait for a +1 from me, unless there's something specific you want input on. I'll trust Andres' review :)

andresmgot

Thanks, still +1 but I would change the test.

andresmgot · 2021-01-27T09:37:10Z

dashboard/src/components/Catalog/Catalog.test.tsx

+    spyOnUseState.mockRestore();
+    expect(setPage).toHaveBeenCalledWith(2); // changes page to 2
+    expect(resetRequestCharts).toHaveBeenCalled(); // but receives a change in a filter
+    expect(wrapper.find("CatalogItems").prop("page")).toBe(1); // and page is again 1


I believe this is not ensuring what you want. You can verify it commenting the useEffect(...) that reset the page and the charts. If you do that, this test will still pass I believe.

I would do a much simpler test, without the setState mock. I would just check that after the filter change, resetRequestCharts is called twice (I belive it's called once the first time the component is rendered). We would not be checking that the page number is reset but since it's in the same piece of logic it's worth it not to add extra complexity in the test.

I just commented
useEffect(() => { setPage(1); resetRequestCharts();}, ... and the test is not passing. Besides, when moving to the simpler test, it seems resetRequestCharts is called just once (but, I agree with you, it should be twice; I will check it)

As per the offline conversation: setPage is not really changing the page expect(wrapper.find("CatalogItems").prop("page")).toBe(2); ---> false.
Besides, implementing the test for testing this is not that easy. For instance, trying to set a repo search filter with 'checked=true` didn't work.

The agreed decision has been just to add a //TODO here and go ahead with this PR (so that the release can be out asap).

andresmgot · 2021-01-27T09:41:05Z

dashboard/src/components/Catalog/Catalog.test.tsx

@@ -366,7 +366,7 @@ describe("pagination and chart fetching", () => {
      />,
    );
    expect(wrapper.find("CatalogItems").prop("page")).toBe(1);
-    expect(wrapper.find("CatalogItems>CatalogItem>ChartCatalogItem").length).toBe(0);
+    expect(wrapper.find("ChartCatalogItem").length).toBe(0);


btw, just fyi in case you don't know, the difference between using wrapper.find(Component) and wrapper.find("Component") is that the second verifies that there is a component named "Component" while the first ensures that the exact component (the one you are importing in the test file) is present.

You don't need to change this, it's just that I usually do the former since it's more accurate, in case there are two components that are named the same.

Ahhh, good to know! Thanks for the tip!

antgamdia · 2021-01-27T17:56:20Z

Merging finally!!! 🤞

wip on pagination

10664da

antgamdia mentioned this pull request Jan 20, 2021

wip on endless scrolling pagination #2221

Closed

7 tasks

andresmgot reviewed Jan 20, 2021

View reviewed changes

antgamdia added 5 commits January 20, 2021 21:31

Rename var

4b236d2

Change hasFinished logic using api 'totalPages' response

8e01ca3

Pass current page in the payload to avoid duplicates

c9794f4

Update tests

722d61c

Fix failing test

1f22d91

antgamdia marked this pull request as ready for review January 20, 2021 23:40

antgamdia changed the title ~~wip on pagination~~ Infinite scrolling pagination Jan 20, 2021

andresmgot reviewed Jan 21, 2021

View reviewed changes

antgamdia added 5 commits January 21, 2021 18:10

Add test cases in actions/charts

488be67

Add test cases in reducers/charts

788318a

Minor UI bug fix

ed53f2f

- If a filter was not present after a change (i.e., a different ns with different repos) the "catalog is empty" msg appeared

Add more test cases to component/catalog

6fac7ea

andresmgot reviewed Jan 22, 2021

View reviewed changes

Merge branch 'pagination' of https://github.com/antgamdia/kubeapps in…

2adfd09

…to pagination

andresmgot reviewed Jan 22, 2021

View reviewed changes

Andres Martinez Gotor and others added 3 commits January 22, 2021 14:09

My changes

4272a91

Merge pull request #1 from andresmgot/pagination-pair-progr

dbe175a

Pagination pair progr

Minor UI improvements

ee30a19

antgamdia added 4 commits January 22, 2021 17:47

Fix tests cases after major changes

001a867

Fix CatalogItem issue

eddfed9

The previous fix didn't completely solve the issue. Now charts and operators are rendered at the same time during the page 1 invocation. Added corresponding tests.

More tests

ca36790

Ignore ts check issue in test

ca119b6

andresmgot reviewed Jan 25, 2021

View reviewed changes

antgamdia mentioned this pull request Jan 25, 2021

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

Merged

antgamdia added 2 commits January 26, 2021 11:04

Address minor PR comments.

77d2d95

Change the approach in the pagination catalog test cases. Add paginat…

0d3c774

…ion test case.

andresmgot approved these changes Jan 26, 2021

View reviewed changes

Add test case

2234313

andresmgot approved these changes Jan 27, 2021

View reviewed changes

Address PR comments

5261278

antgamdia merged commit 9bb37b5 into vmware-tanzu:master Jan 27, 2021

antgamdia deleted the pagination branch January 27, 2021 17:56

		const lastPendingPage = Array.from(records.keys()).pop() \|\| 1;
		if (records.get(page) === false && page <= lastPendingPage) {

Infinite scrolling pagination #2264

Infinite scrolling pagination #2264

Conversation

antgamdia commented Jan 20, 2021 • edited Loading

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

First render:

Scrolling

andresmgot commented Jan 20, 2021

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgamdia Jan 20, 2021 • edited Loading

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 Jan 20, 2021 • edited Loading

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 20, 2021

andresmgot 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

antgamdia commented Jan 21, 2021

antgamdia commented Jan 22, 2021

Choose a reason for hiding this comment

antgamdia Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgamdia Jan 22, 2021 • edited Loading

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

andresmgot commented Jan 22, 2021 • edited Loading

andresmgot commented Jan 22, 2021

antgamdia commented Jan 22, 2021

antgamdia commented Jan 22, 2021

antgamdia commented Jan 22, 2021

First render:

Scrolling

andresmgot 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

antgamdia Jan 26, 2021 • edited Loading

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 20, 2021 •

edited

Loading

antgamdia Jan 20, 2021 •

edited

Loading

antgamdia Jan 20, 2021 •

edited

Loading

antgamdia Jan 22, 2021 •

edited

Loading

antgamdia Jan 22, 2021 •

edited

Loading

andresmgot commented Jan 22, 2021 •

edited

Loading

antgamdia Jan 26, 2021 •

edited

Loading

andresmgot Jan 25, 2021 •

edited

Loading