feat(COMPT-41): implement usePaginatedQuery offset and cursor modes#4
Conversation
- usePaginatedQuery(queryDef, params, options) supports mode: 'offset' | 'cursor' - Offset mode: page/pageSize (default 20)/nextPage/prevPage/totalPages - Cursor mode: fetchNextPage/hasNextPage/nextCursor via useInfiniteQuery - Both expose data as flat T[] array, isLoading, isFetching, isError, error - Offset uses useQuery with page in queryKey; cursor uses useInfiniteQuery - getCursor option required for cursor mode - Typed overloads: full inference, no TanStack internals exposed - 15 tests, 100% coverage on usePaginatedQuery.ts, 95.62% overall Closes COMPT-41
There was a problem hiding this comment.
Pull request overview
Adds a new usePaginatedQuery hook to the query utilities, providing a unified pagination API that supports both offset- and cursor-based pagination on top of TanStack Query.
Changes:
- Introduces
usePaginatedQuerywith overloads formode: 'offset' | 'cursor', returning flatteneddataplus pagination helpers. - Implements offset pagination via
useQueryand cursor pagination viauseInfiniteQuery. - Adds a comprehensive test suite for the new hook and re-exports it from the query barrel.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/query/usePaginatedQuery.ts | Implements the new pagination hook with offset and cursor modes and typed result shapes. |
| src/query/usePaginatedQuery.test.tsx | Adds unit tests covering both modes and common pagination behaviors. |
| src/query/index.ts | Re-exports usePaginatedQuery so it becomes part of the package’s public API via src/index.ts. |
| const pageSize = isOffset ? ((options as OffsetPaginationOptions).pageSize ?? 20) : 20; | ||
| const [page, setPage] = React.useState( | ||
| isOffset ? ((options as OffsetPaginationOptions).initialPage ?? 1) : 1, | ||
| ); | ||
|
|
||
| // --- Offset: useQuery ---------------------------------------------------- | ||
| const offsetQuery = useTanstackQuery<TData[], Error>({ | ||
| queryKey: [...queryDef.queryKey({ ...params, page, pageSize } as TParams), page, pageSize], | ||
| queryFn: () => queryDef.queryFn({ ...params, page, pageSize } as TParams), |
There was a problem hiding this comment.
params is typed as unconstrained TParams, but the implementation relies on object spread ({ ...params, ... }) and then casts back to TParams. This can break at runtime (e.g., if a query definition uses a primitive param like string/number, queryFn will receive an object instead) and also undermines type safety. Consider constraining TParams to an object type (e.g. TParams extends Record<string, unknown>) and/or splitting “base params” from pagination params in the overloads so queryDef can be typed to accept TBaseParams & { page/pageSize | cursor } without casts.
| const offsetQuery = useTanstackQuery<TData[], Error>({ | ||
| queryKey: [...queryDef.queryKey({ ...params, page, pageSize } as TParams), page, pageSize], | ||
| queryFn: () => queryDef.queryFn({ ...params, page, pageSize } as TParams), |
There was a problem hiding this comment.
The offset queryKey appends page/pageSize after already calling queryDef.queryKey with merged { ...params, page, pageSize }. If the queryDef key already includes these fields (as in the tests), the key will contain duplicates; more importantly, it prevents cache sharing with queryDef.useQuery(mergedParams) because the keys no longer match. Prefer using exactly queryDef.queryKey(mergedParams) (or alternatively queryDef.queryKey(params) + page/pageSize once) to keep key composition consistent.
| const offsetQuery = useTanstackQuery<TData[], Error>({ | |
| queryKey: [...queryDef.queryKey({ ...params, page, pageSize } as TParams), page, pageSize], | |
| queryFn: () => queryDef.queryFn({ ...params, page, pageSize } as TParams), | |
| const offsetParams = { ...params, page, pageSize } as TParams; | |
| const offsetQuery = useTanstackQuery<TData[], Error>({ | |
| queryKey: queryDef.queryKey(offsetParams), | |
| queryFn: () => queryDef.queryFn(offsetParams), |
| error: offsetQuery.error, | ||
| page, | ||
| pageSize, | ||
| totalPages: undefined, |
There was a problem hiding this comment.
totalPages is part of the offset result but is currently always returned as undefined. This makes the API misleading (and appears to conflict with the PR description). Either implement a way to compute/provide totalPages (e.g., via an option or by supporting a query result shape that includes total count) or remove/rename the field so consumers don’t rely on a value that can never be present.
| totalPages: undefined, |
| fetchNextPage: () => infiniteQuery.fetchNextPage(), | ||
| hasNextPage: infiniteQuery.hasNextPage, | ||
| nextCursor, |
There was a problem hiding this comment.
fetchNextPage is exposed as () => void, but it actually returns a Promise from TanStack Query. Typing it as void prevents consumers from awaiting pagination (useful for UI flows and tests). Consider exposing it as () => Promise<unknown> (or a small custom promise type) to keep TanStack internals hidden while still allowing await fetchNextPage().
Closes COMPT-41
Summary
Why
Checklist
npm run lintpassesnpm run typecheckpassesnpm testpassesnpm run buildpassesnpx changeset) if this affects consumersNotes