#60 implement infinite scroll by MainlyColors · Pull Request #90 · windyScripts/dev-directory

MainlyColors · 2023-04-19T21:41:25Z

Closes #60

Description

feature

Use Intersection Observer to trigger loading of a new batch of 20 profiles on the /directory page when the user scrolls down to the height of the last card above the bottom of the window/ul that contains all the users
Will not load pages past the total pages returned by /api/users
only triggers the load event once because the observer is removed on first intersection
A loading spinner is triggered if the data doesn't load instantly

Tests added

cypress tests added for all functionality
seed script is triggered before all tests in the directory infinite scroll test suite to generate 100 users

Testing

manually

start docker container
npm run dev:server
npm run seed - to generate a 100 users -
visit \directory
- keep in mind currently discord_name has a 20% chance of being empty which is being fixed in another PR but will cause some users not to show up on the page
scroll down the page to load a new 20 users (repeat until no more users)
in the network tab can change the throttle to slow 3G to catch the loading spinner

Automatic

start docker container
npm run dev:server
npm run cypress:open
click "Start E2E Test" with Chrome
Tests will run and should all pass

Type of change

New feature (non-breaking change which adds functionality)

Screenshots

Tests passing - directory.spec.ts

Known issue - intercepting user api w/ cypress

The use of cypress intercept here in the directory test is intended to wait upon the response and catch the loading spinner. This works 95-99% of the time but if the loading spinner appears for less than 20ms then cypress won't be able to catch it, resulting in a failed test. Just re-run the test to be sure.

Different variations of using as(), (req) => req.alias, and even req.reply(res => res.setDelay(100)) and every possible permutation of those 3 were tried by Myself and Devon but we still got random errors.

it('shows a loading animation while fetching users', () => {
    cy.scrollTo('bottom');
    cy.intercept('/api/users?page=*').as('getUsers');

    // spinner should exist & be removed after request completes
    cy.get('[data-cy="loading"]');
    cy.wait('@getUsers');
    cy.get('[data-cy="loading"]').should('not.exist');
  });

Checklist:

Changes have new/updated automated tests, if applicable
Changes have new/updated docs, if applicable
I have performed a self-review of my own code
I have added comments on any new, hard-to-understand code
Changes have been manually tested
Changes generate no new errors or warnings

MainlyColors · 2023-04-19T21:54:49Z

+    cy.intercept('GET', '/api/users?page=*', req => {
+      req.on('response', res => res.setDelay(500));
+    }).as('getUsers');


intercepts the api call to add a 500 ms delay to give cypress enough time to see the loading spinner

devonzara

Apologies for the delay... I've not gotten the chance to pull it down and actually work through/test the PR, but I've added some comments/suggested changes that can be done in the interim. 👍🏼

devonzara · 2023-05-13T20:50:03Z

+  env: {
+    bash: 'C:\\Program Files\\Git\\bin\\bash.exe',
+    comSpec: 'C:\\Windows\\system32\\cmd.exe',
+  },


This is no longer needed as part of this PR.

devonzara · 2023-05-13T20:51:49Z

+  before(() => {
+    // clear Db
+    cy.truncateDatabase();
+
+    // fill DB with 100 users
+    cy.createUsers(100);
+
+    // get total pages for tests
+    cy.request('http://localhost:3000/api/users').then(response => {
+      totalPages = response.body.totalPages;
+    });


I'd probably remove these 3 comments as they don't really add any additional information.

devonzara · 2023-05-13T20:53:57Z

+      i < totalPages
+        ? cy.get('@ulChildren').should('have.length', PAGE_LIMIT * i)
+        : // last page not guaranteed to be full
+        cy.get('@ulChildren').should('have.length.within', PAGE_LIMIT * (i - 1), PAGE_LIMIT * i);


nit: formatting

Suggested change

i < totalPages

? cy.get('@ulChildren').should('have.length', PAGE_LIMIT * i)

: // last page not guaranteed to be full

cy.get('@ulChildren').should('have.length.within', PAGE_LIMIT * (i - 1), PAGE_LIMIT * i);

i < totalPages ?

cy.get('@ulChildren').should('have.length', PAGE_LIMIT * i) :

// last page not guaranteed to be full

cy.get('@ulChildren').should('have.length.within', PAGE_LIMIT * (i - 1), PAGE_LIMIT * i);

devonzara · 2023-05-13T21:00:50Z

+      // last page won't have a spinner
+      if (i === totalPages) break;
+      // check spinner happened & removed
+      cy.get('[data-cy="loading"]'); //check if exists
+      cy.get('[data-cy="loading"]').should('not.exist'); // removed after data loaded


nit: formatting, empty line after breaks/continues & same line comments should typically be avoided

Suggested change

// last page won't have a spinner

if (i === totalPages) break;

// check spinner happened & removed

cy.get('[data-cy="loading"]'); //check if exists

cy.get('[data-cy="loading"]').should('not.exist'); // removed after data loaded

// last page won't have a spinner

if (i === totalPages) break;

// spinner should exist & be removed after request completes

cy.get('[data-cy="loading"]');

cy.get('[data-cy="loading"]').should('not.exist');

Also, is this missing a cy.wait or something? How does it know when the request is complete?

@devonzara Cypress will keep retrying their commands for 4 secs each before it declares it failed. That's why a cy.wait isn't required

devonzara · 2023-05-13T21:01:13Z

+    cy.get('[data-cy="loading"]'); //check if exists
+    cy.wait('@getUsers');
+    cy.get('[data-cy="loading"]').should('not.exist'); // removed after data loaded


nit: same as above, inline comments

devonzara · 2023-05-13T21:05:45Z

+        users: response.data.users,
+      };
+    } catch (err) {
+      console.error(err);


We should avoid using console on the client side as it will fail without notifying the user... see linked comments on #87 and the approach they took to address it.

#87 (comment)
#87 (comment)

devonzara · 2023-05-13T21:07:55Z

+        setTotalPageNum(total);
+        setUserData([...userData, ...users]);
+      } catch (err) {
+        console.error(err);


Same as above, we should avoid using console in the client-side code.

timmyichen

minor comments, nothing major, i think this is almost ready to go!

timmyichen · 2023-05-20T07:18:40Z

+      i < totalPages ?
+        cy.get('@ulChildren').should('have.length', PAGE_LIMIT * i) :
+        // last page not guaranteed to be full
+        cy.get('@ulChildren').should('have.length.within', PAGE_LIMIT * (i - 1), PAGE_LIMIT * i);


stylistic choice, but we should reserve ternary statements for values, rather than actual execution. an if/else would be preferred here

timmyichen · 2023-05-20T07:21:06Z

+    return { users, totalPages, error: null };
  } catch (error) {
-    return { users: [], error: error.message };
+    return { users: [], totalPages: 1, error: error.message };


why is totalPages set to 1 here?

I set it to 1 because if there are no users then there would only be 1 page and we set up the users API to throw an error on calling page=0

timmyichen · 2023-05-20T07:22:36Z

+  const [currentPage, setCurrentPage] = React.useState(1);
+  const [totalPageNum, setTotalPageNum] = React.useState(totalPages);
+  const [isLoading, setIsLoading] = React.useState(false);
+  const [open, setOpen] = React.useState(false);


would prefer to have this be named a little more specifically (if we have more than one alert/modal/etc, what this opens becomes ambiguous). showError, setShowError as a sample alternative

timmyichen · 2023-05-20T07:24:02Z

+        if (currentPage + 1 > totalPageNum) {
+          setIsLoading(false);
+          return;
+        }


can this check happen before we even set isLoading? that way we can just setIsLoading(true) right before we start fetching the next page of users

timmyichen · 2023-05-20T07:25:11Z


  return (
    <Container maxWidth="lg" className="pt-4">
+      <ErrorPopup open={open} setOpen={setOpen} message="Page out of range" />


A note - Page out of range isn't the only possible error we could get - but I think error handling in general can be worked on later so I won't block on this :P

timmyichen · 2023-05-20T07:25:51Z

+      }
+    }, intersectionOptions);
+
+    setIsLoading(false);


was this meant to be inside of the observer callback?

Originally I put it there for a reason I can't remember, but within the observer callback makes more sense 👍

devonzara

Looking good, mostly nits and questions about error handling remain.

devonzara · 2023-05-22T20:55:32Z

nit: I think this might be better named ErrorToast or even just Toast as Popup is a bit ambiguous and Error limits the scope of its future use.

I agree on ErrorToast for now, I think configuring the component to handle situations that haven't occurred yet and may never occur goes against YAGNI. In the future, if we need the toast to do more then we can add the functionality then 😎

devonzara · 2023-05-22T20:56:25Z

+
+      cy.intercept('GET', '/api/users?page=*', req => {
+        req.on('response', res => res.setDelay(500));
+      }).as('getUsers');


nit: is the .as() alias necessary if we don't have to wait for it?

While researching about .as() more and if cy.wait() was necessary, I found the correct way to actually pause the response while waiting for the loading spinner

cy.intercept('/api/users?page=*', req => { /** * If set, `cy.wait` can be used to await the request/response * cycle to complete for this request via `cy.wait('@alias')`. */ req.alias = 'getUsers'; }).as('getUsers'); cy.get('[data-cy="loading"]'); // here cypress releases the response cy.wait('@getUsers'); cy.get('[data-cy="loading"]').should('not.exist');

docs: https://docs.cypress.io/api/commands/intercept#Request-object-properties

@MainlyColors Correct me if I'm wrong, but I believe .as() and req.alias = have similar behaviors, and are therefore redundant when used together. Given that dynamic alias assignment is not a requirement in this context, using .as() alone should suffice. No?

devonzara · 2023-05-22T20:57:37Z

+
+    // spinner should exist & be removed after request completes
+    cy.get('[data-cy="loading"]');
+    cy.wait('@getUsers');


Is this cy.wait necessary? If so, what's different here vs the previous test? If cy.wait isn't necessary in either case, can we get rid of it and the .as() alias above?

devonzara · 2023-05-22T20:58:07Z

+          message: 'Request failed with status code 400',
+        },
+      });
+      // req.on('before:response', res => (res.statusCode = 400));


nit: lingering line of commented code

devonzara · 2023-05-22T20:59:17Z

+        },
+      });
+      // req.on('before:response', res => (res.statusCode = 400));
+    }).as('getUsers');


nit: unused .as() alias

devonzara · 2023-05-22T21:02:55Z

+        users: response.data.users,
+      };
+    } catch (err) {
+      if (err.code === 'ERR_BAD_REQUEST') {


What happens if other errors occur?

I didn't know all the errors that were possible, Ill look into it more

Every possible error doesn't need to be explicitly handled; the user doesn't always need to know what the error was.

Some errors may be network related and work themselves out, others might be things we need to address... either way, communicating that an error has occurred to the user instead of leaving them uncertain results in a better user experience.

devonzara · 2023-05-22T21:05:46Z

+        setTotalPageNum(total);
+        setUserData([...userData, ...users]);
+      } catch (err) {
+        if (err.message === 'Page out of range') {


I'm not sure this conditional is necessary... or, if kept - should we provide an else condition with a generic error?

Caleb-Cohen

LGTM

devonzara

LGTM. Thanks for taking this on!

MainlyColors added 10 commits April 17, 2023 12:55

add simiple UI for /directory user cards to test infinite scoll

c11f459

get infinite scroll working

1670a4c

relocate setIsLoading

4610b53

remove UI changes that were used only for testing

63ff41d

remove mui imports that were being used for UI testing

68e4bf5

remove console.log

ca3db0b

remove unobserve in favor of disconnect method

619dce5

relocate observer.disconnect to remove observer immediately

477eae7

revert offset back to 300px

6f4352e

Add cypress tests for infinite scroll

b1f1fb1

MainlyColors requested review from Caleb-Cohen, DevinCLane, MatthewBozin, code2244, dev-skworks, devonzara, intelagense, jamiecha88, jmoldyvan, sstrunks52005, timmyichen, windyScripts and wo1vin as code owners April 19, 2023 21:41