feat: add usePaginatedQuery hook by buenos-nachos · Pull Request #10803 · coder/coder

buenos-nachos · 2023-11-20T19:36:32Z

Closes #10589 (well 3/4 of the matters from the issue – auto-scrolling will be handled in a separate PR)

Changes made

Adds a custom usePaginatedQuery hook, as well as associated utility types. The hook handles:
- Managing and syncing the page params in the URL search bar while limiting how much you need to wire things up yourself
- Prefetches for the previous and next pages when available
- Page transitions if you accidentally try to navigate to invalid pages
Adds 20 test cases for the hook
Integrates the hook into two of our main paginated tables (users, audits). The only one not touched is the Workspaces table.
- The Workspaces table will require more work, just because so many other ad-hoc custom hooks are coupled to the current implementation
- These two tables do have query caching turned on, though, so navigating pages should be much faster
Adds function overloads to the prepareQuery helper function to give it more specific return type info
Made a tiny fix to useEffectEvent to make sure that it still works properly if you use it with useLayoutEffect

Notes

The usePaginatedQuery file is long – over 400 lines of code, with 1/3 runtime logic, 1/3 type definitions, and 1/3 comments to explain the type definitions. You might want to save it for last
- The file really got into the weeds of TypeScript's type system, and while I tried my best to make things as clear as possible, please flag anything that's remotely confusing. I'm a believer that if something was hard to write, it's going to be harder to read – and I had to consult the TS Wizards Discord to get the type definitions finished up and out the door
I'm going to delay merging this in until I have the auto-scrolling PR ready to go, just so the UI isn't stuck in a janky in-between period because it wasn't built to account for cached query data
I felt like I was running into weird edge cases with React Router's useSearchParams hook and how it gives you new values every single time you call it. Other parts of the codebase are jumping through weird hoops to get around it, too, so I'll see if there's a safe way to make the values truly shared via context

buenos-nachos · 2023-11-22T17:45:18Z

-    queryKey: ["users", req],
+    queryKey: usersKey(req),
    queryFn: ({ signal }) => API.getUsers(req, signal),
+    cacheTime: 5 * 1000 * 60,


This function is still being used in one other place that doesn't require paginated data, so I can't get rid of it just yet

I'm trying to review kind of quickly since I'm on a time crunch this morning and have made you wait a couple days, but I just wanna make sure that this aligns with the caching stuff @BrunoQuaresma said he's doing

Just double-checked, and the caching PR has been committed, so these can be safely removed. There wouldn't be any conflicts – they'd just be redundant

aslilac · 2023-11-29T21:11:28Z

+    },
+
+    cacheTime: 5 * 1000 * 60,
+  } as const satisfies UsePaginatedQueryOptions<AuditLogResponse, string>;


I'd maybe just put this as the return type rather that use satisfies

aslilac · 2023-11-29T21:12:45Z

-    queryKey: ["users", req],
+    queryKey: usersKey(req),
    queryFn: ({ signal }) => API.getUsers(req, signal),
+    cacheTime: 5 * 1000 * 60,


I'm trying to review kind of quickly since I'm on a time crunch this morning and have made you wait a couple days, but I just wanna make sure that this aligns with the caching stuff @BrunoQuaresma said he's doing

aslilac · 2023-11-29T21:14:38Z

+  jest.clearAllMocks();
+});
+
+function render<


hooks don't really get "rendered" so to speak, so this name feels a bit weird. maybe it could be called testPaginatedQuery or something? not a huge deal tho

aslilac · 2023-11-29T21:15:20Z

+ * There are a lot of test cases in this file. Scoping mocking to inner describe
+ * function calls to limit the cognitive load of maintaining all this stuff
+ */
+describe(`${usePaginatedQuery.name} - Overall functionality`, () => {


nit: I feel like "overall functionality" is to be assumed if you're not mentioning something more specific

aslilac · 2023-11-29T21:17:54Z

+      const firstPageOptions = getQueryOptionsFromPage(1);
+      try {
+        const firstPageResult = await queryClient.fetchQuery(firstPageOptions);
+        fixedTotalPages = Math.ceil(firstPageResult?.count ?? 0 / limit) || 1;


Suggested change

fixedTotalPages = Math.ceil(firstPageResult?.count ?? 0 / limit) || 1;

fixedTotalPages = Math.max(Math.ceil(firstPageResult?.count ?? 0 / limit), 1);

has a slightly different meaning, but if it wouldn't break anything I think the intent is clearer

aslilac · 2023-11-29T21:18:24Z

+      try {
+        const firstPageResult = await queryClient.fetchQuery(firstPageOptions);
+        fixedTotalPages = Math.ceil(firstPageResult?.count ?? 0 / limit) || 1;
+      } catch (err) {


Suggested change

} catch (err) {

} catch {

no need to capture the error as a variable

Oh, I had no idea that you could have a catch block without the error definition. I'll try not to abuse that too much

aslilac · 2023-11-29T21:18:58Z

+    }
+
+    const withoutPage = getParamsWithoutPage(searchParams);
+    if (onInvalidPageChange === undefined) {


Suggested change

if (onInvalidPageChange === undefined) {

if (!onInvalidPageChange) {

aslilac · 2023-11-29T21:24:33Z

+    }
+
+    const cleanedInput = clamp(Math.trunc(newPage), 1, totalPages ?? 1);
+    if (!Number.isInteger(cleanedInput) || cleanedInput <= 0) {


what makes these checks necessary? the trunc and clamp should already guarantee these already?

There's still one check that needs to be done for NaN values – they won't throw errors for any of the math calculations

But you're right – one of the checks doesn't apply anymore, and the other can just turn into Number.isNaN

aslilac · 2023-11-29T21:28:15Z

+  // useLayoutEffect should be overkill here 99% of the time, but it ensures it
+  // will run before any other layout effects that need this custom hook


eh, you're just describing what useLayoutEffect is for. maybe describe why it matters that this runs as a layout effect instead.

Yeah, good point

aslilac · 2023-11-29T21:32:20Z

+export function prepareQuery(query: undefined): undefined;
+export function prepareQuery(query: string): string;
+export function prepareQuery(query?: string): string | undefined {


Suggested change

export function prepareQuery(query: undefined): undefined;

export function prepareQuery(query: string): string;

export function prepareQuery(query?: string): string | undefined {

export function prepareQuery<T extends string | undefined>(query: T): T extends undefined ? string | undefined : string {

as this is defined, I believe you'd actually be unable to pass a value of type string | undefined. could be "simplified"¹ a bit to clear things up

Footnotes

type parameters are not simple but neither is "overloading" ↩

So, I'm having trouble getting the compiler to be happy with your suggestion

I modified the overload signature to accept string | undefined values, but at this point, yeah, probably better to refactor this to have better type signatures down the line

buenos-nachos added 22 commits November 13, 2023 21:52

wip: commit current progress on usePaginatedQuery

b4211f3

chore: add cacheTime to users query

173e823

chore: update cache logic for UsersPage usersQuery

d715d73

wip: commit progress on Pagination

8a199b7

chore: add function overloads to prepareQuery

98ae96d

wip: commit progress on usePaginatedQuery

37ea50b

docs: add clarifying comment about implementation

2c1e9e3

chore: remove optional prefetch property from query options

b3a9ab4

chore: redefine queryKey

39a2ced

refactor: consolidate how queryKey/queryFn are called

4dcfd29

refactor: clean up pagination code more

86f8437

fix: remove redundant properties

f612d8f

refactor: clean up code

5878326

wip: commit progress on usePaginatedQuery

5cc1c2d

wip: commit current pagination progress

0138f21

docs: clean up comments for clarity

a38fd30

wip: get type signatures compatible (breaks runtime logic slightly)

22d6c24

refactor: clean up type definitions

e717da1

chore: add support for custom onInvalidPage functions

7624d94

refactor: clean up type definitions more for clarity reasons

2623131

chore: delete Pagination component (separate PR)

29242e9

chore: remove cacheTime fixes (to be resolved in future PR)

5a9aa2d

buenos-nachos self-assigned this Nov 20, 2023

docs: add clarifying/intellisense comments for DX

3d67304

buenos-nachos changed the title ~~feat: add usePaginationQuery hook~~ feat: add usePaginatedQuery hook Nov 20, 2023

buenos-nachos added 5 commits November 20, 2023 20:23

refactor: link users queries to same queryKey implementation

d977060

docs: remove misleading comment

9224624

docs: more comments

540c779

chore: update onInvalidPage params for more flexibility

4b42b6f

fix: remove explicit any

5bc43c9

buenos-nachos added 17 commits November 21, 2023 18:05

fix: remove deprecated properties from hook

218da68

refactor: clean up code more

54f01f1

docs: add todo comment

ede2abc

chore: update testing fixtures

9ecab16

wip: commit current progress for tests

0c81d1c

fix: update useEffectEvent to sync via layout effects

3aa714c

wip: commit more progress on tests

2893630

wip: stub out all expected test cases

ef900d4

wip: more test progress

23dc583

wip: more test progress

24361d1

wip: commit more test progress

755f8c0

wip: AHHHHHHHH

8bff0d3

chore: finish two more test cases

1fae773

wip: add in all tests (still need to investigate prefetching

be82728

refactor: clean up code slightly

848fa0f

fix: remove math bugs when calculating pages

c89e8e3

fix: wrap up all testing and clean up cases

3624a6d

buenos-nachos requested review from a team and aslilac and removed request for a team November 22, 2023 17:40

buenos-nachos marked this pull request as ready for review November 22, 2023 17:40

buenos-nachos commented Nov 22, 2023

View reviewed changes

buenos-nachos added 2 commits November 22, 2023 17:53

docs: update comments for clarity

13e2f30

fix: update error-handling for invalid page handling

8f83673

buenos-nachos mentioned this pull request Nov 28, 2023

fix: create centralized Pagination component #10911

Closed

aslilac approved these changes Nov 29, 2023

View reviewed changes

fix: apply suggestions

5fb0643

buenos-nachos merged commit d016f93 into main Nov 30, 2023

buenos-nachos deleted the mes/pagination-2 branch November 30, 2023 22:44

github-actions Bot locked and limited conversation to collaborators Nov 30, 2023

	fixedTotalPages = Math.ceil(firstPageResult?.count ?? 0 / limit) \|\| 1;
	fixedTotalPages = Math.max(Math.ceil(firstPageResult?.count ?? 0 / limit), 1);

	if (onInvalidPageChange === undefined) {
	if (!onInvalidPageChange) {

		// useLayoutEffect should be overkill here 99% of the time, but it ensures it
		// will run before any other layout effects that need this custom hook

Conversation

buenos-nachos commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes made

Notes

Uh oh!

buenos-nachos Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buenos-nachos Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

buenos-nachos commented Nov 20, 2023 •

edited

Loading

buenos-nachos Nov 22, 2023 •

edited

Loading

buenos-nachos Nov 30, 2023 •

edited

Loading