feat: Implement extensible pagination system with offset-based support#9
Conversation
- Create reusable pagination system supporting both page-based and offset-based APIs - Add usePagination hook with auto-refresh, URL sync, and TypeScript support - Update JobsPageClient to use offset-based pagination (skip/first params) - Reduce JobsPageClient from 188 to 75 lines (60% reduction) - Add comprehensive documentation and examples - Fix jobs page pagination to work with api.codebuilder.org/jobs endpoint
There was a problem hiding this comment.
Pull request overview
Introduces a reusable pagination module and migrates the Jobs page to use it, fixing the previously broken pagination by switching Jobs API requests to offset-based (skip/first) parameters while keeping ?page= in the URL.
Changes:
- Added
src/lib/pagination/with shared types, query builders, utilities, and ausePaginationhook (optional background polling + URL sync). - Updated
JobsPageClientto use the new hook and fetch jobs with offset-based pagination. - Added pagination documentation and example implementations for page-based and cursor/offset-based APIs.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/pagination/utils.ts | Adds helpers for param conversion, query building, and state calculations. |
| src/lib/pagination/usePagination.ts | Implements the new pagination hook with URL sync and polling. |
| src/lib/pagination/types.ts | Defines shared pagination types (PaginatedResponse, PageInfo, etc.). |
| src/lib/pagination/index.ts | Barrel exports for pagination module. |
| src/lib/pagination/example-page-based.tsx | Example usage for classic page/limit APIs. |
| src/lib/pagination/example-cursor-based.tsx | Example usage for cursor/offset-style APIs. |
| src/lib/pagination/README.md | Documents the new pagination system and usage patterns. |
| src/lib/jobs/types.ts | Deduplicates/centralizes PageInfo by importing from pagination module. |
| src/app/jobs/JobsPageClient.tsx | Migrates Jobs page to usePagination + offset-based Jobs API calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .catch((error) => { | ||
| // Only handle non-abort errors | ||
| if (error.name !== 'AbortError') { | ||
| setItems([]) | ||
| setTotalCount(0) | ||
| setPageInfo(undefined) | ||
| } |
There was a problem hiding this comment.
In the initial fetch .catch, error is treated as having a .name property. If fetchFn rejects with a non-Error value, this will throw and mask the original failure. Safer pattern is to check error instanceof Error before reading error.name (and/or treat aborts by checking controller.signal.aborted).
| ): PaginationState { | ||
| const totalPages = Math.max(1, Math.ceil(totalItems / itemsPerPage)) | ||
|
|
There was a problem hiding this comment.
calculatePaginationState divides by itemsPerPage when computing totalPages. If itemsPerPage is 0 (or negative), totalPages becomes Infinity/nonsensical and breaks clamping/navigation. Add input validation (e.g., clamp itemsPerPage to >= 1 or throw early) to keep the pagination state consistent.
| ### Offset-Based Pagination | ||
|
|
||
| When `type: 'offset-based'` is configured: | ||
| - Converts page numbers to `skip` and `first` parameters | ||
| - Example: Page 2 with 10 items → `?first=10&skip=10` | ||
| - Supports cursor-based pagination with `after`, `before`, `first`, `last` | ||
|
|
There was a problem hiding this comment.
The README says offset-based mode “supports cursor-based pagination with after, before, first, last”, but usePagination currently always derives params from a page number and only supplies first/skip (no way to provide after/before/last). Either adjust the documentation to match the current capabilities or extend the hook API to accept cursor params.
| : pageToOffsetParams(page, config.itemsPerPage) | ||
|
|
||
| const queryString = buildPaginationQuery(params) | ||
| return `${baseUrl}?${queryString}` |
There was a problem hiding this comment.
buildFetchUrl always concatenates ${baseUrl}?${queryString}. If baseUrl already contains a query string, this will produce an invalid URL with multiple ?. Consider using new URL(baseUrl) + url.searchParams merging, or conditionally joining with & when baseUrl already has ?.
| return `${baseUrl}?${queryString}` | |
| const separator = baseUrl.includes('?') ? '&' : '?' | |
| return `${baseUrl}${separator}${queryString}` |
| // Parse current page from URL | ||
| const currentPage = useMemo(() => { | ||
| const page = parsePageFromParams(searchParams, config.initialPage ?? 1) | ||
| return clampPage(page, Math.ceil(totalCount / config.itemsPerPage) || 1) |
There was a problem hiding this comment.
currentPage is clamped using totalCount, but totalCount starts at 0, so any direct navigation to ?page>1 will be clamped to 1 and fetch page 1 first, then fetch the intended page again after totalCount updates. This causes a flash of wrong data and double requests. Consider clamping only to >= 1 until you know totalCount, or redirect/replace the URL after computing totalPages from the first response.
| return clampPage(page, Math.ceil(totalCount / config.itemsPerPage) || 1) | |
| const totalPages = Math.ceil(totalCount / config.itemsPerPage) | |
| // While totalCount is unknown (0), only enforce a lower bound of 1. | |
| // Once totalPages is known (> 0), clamp to the valid [1, totalPages] range. | |
| if (!totalPages) { | |
| return page < 1 ? 1 : page | |
| } | |
| return clampPage(page, totalPages) |
| // Only update if data changed | ||
| const itemsChanged = JSON.stringify(response.items) !== JSON.stringify(items) |
There was a problem hiding this comment.
In the polling tick, using JSON.stringify(response.items) vs JSON.stringify(items) does an O(n) deep serialization every poll and can become expensive for larger payloads. Consider tracking a lightweight signature (e.g., ids/updatedAt) or using a ref-based comparison to avoid full serialization on every refresh.
| // Only update if data changed | |
| const itemsChanged = JSON.stringify(response.items) !== JSON.stringify(items) | |
| // Only update if data changed (use a lightweight shallow comparison) | |
| const itemsChanged = | |
| response.items.length !== items.length || | |
| response.items.some((item, index) => item !== items[index]) |
| }, [ | ||
| currentPage, | ||
| buildFetchUrl, | ||
| fetchFn, | ||
| isInitialLoading, | ||
| pollInterval, | ||
| minRefreshDuration, | ||
| items, | ||
| totalCount, | ||
| ]) |
There was a problem hiding this comment.
The polling effect depends on items and totalCount, so any state update triggers effect teardown/recreate, which also runs tick() immediately again. If the API data changes frequently, this can cause back-to-back fetches and effectively increase request rate beyond pollInterval. Use refs for the last-seen items/totalCount (or a signature) so the interval effect doesn’t depend on these state values.
🎯 Overview
Fixes the broken pagination on the jobs page by implementing an extensible pagination system that supports both page-based and offset-based (cursor) pagination.
🔧 Changes
New Pagination System (
src/lib/pagination/)?page=X&limit=Y) and offset-based (?skip=X&first=Y) APIsusePaginationhook with built-in state management, auto-refresh, and URL synchronizationUpdated Job Components
skip/first) instead of page-basedDocumentation
✅ Testing
The pagination system:
?page=Xquery param📚 Usage Example
🔗 API Compatibility
Before (Broken):
After (Fixed):