fix: cap pagination to maximum fetchable results#2205
fix: cap pagination to maximum fetchable results#2205thealxlabs wants to merge 3 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request addresses pagination display issues by introducing a Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e52a54dc-e895-4fa3-935d-6af31767c168
📒 Files selected for processing (2)
app/pages/search.vuetest/unit/app/utils/pagination.spec.ts
| const paginationTotal = computed(() => { | ||
| const cap = EAGER_LOAD_SIZE[searchProvider.value] | ||
| return Math.min(effectiveTotal.value, cap) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Finding PaginationControls implementation..."
files="$(fd 'PaginationControls.*\.vue$' app || true)"
printf '%s\n' "$files"
if [ -n "$files" ]; then
echo
echo "Checking for internal clamping logic in PaginationControls..."
rg -n -C3 'currentPage|maxPage|Math\.ceil|totalItems|total-items|watch\(' $files
fi
echo
echo "Checking page initialisation/clamping in search page..."
rg -n -C3 'initialPage|currentPage|paginationTotal|maxFetchablePages|Math\.ceil|watch\(' app/pages/search.vueRepository: npmx-dev/npmx.dev
Length of output: 7069
Clamp currentPage against capped max pages to cover deep-link edge cases.
URL-derived currentPage can exceed the capped pagination range (e.g., ?page=999 from stale links). Whilst PaginationControls clamps page changes via its internal goToPage() guard, the initial currentPage value set on mount from the URL bypasses this check, resulting in an invalid state where the displayed pagination text and item range become incoherent (e.g., "Showing items 24976 to 500" when total is capped at 500).
Add a watch to clamp currentPage when paginationTotal or preferredPageSize changes:
Suggested clamp in app/pages/search.vue
const paginationTotal = computed(() => {
const cap = EAGER_LOAD_SIZE[searchProvider.value]
return Math.min(effectiveTotal.value, cap)
})
+
+const maxFetchablePages = computed(() =>
+ Math.max(1, Math.ceil(paginationTotal.value / preferredPageSize.value)),
+)
+
+watch([maxFetchablePages, currentPage], ([maxPages, page]) => {
+ if (page > maxPages) {
+ currentPage.value = maxPages
+ updateUrlPage(maxPages)
+ }
+})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const paginationTotal = computed(() => { | |
| const cap = EAGER_LOAD_SIZE[searchProvider.value] | |
| return Math.min(effectiveTotal.value, cap) | |
| }) | |
| const paginationTotal = computed(() => { | |
| const cap = EAGER_LOAD_SIZE[searchProvider.value] | |
| return Math.min(effectiveTotal.value, cap) | |
| }) | |
| const maxFetchablePages = computed(() => | |
| Math.max(1, Math.ceil(paginationTotal.value / preferredPageSize.value)), | |
| ) | |
| watch([maxFetchablePages, currentPage], ([maxPages, page]) => { | |
| if (page > maxPages) { | |
| currentPage.value = maxPages | |
| updateUrlPage(maxPages) | |
| } | |
| }) |
| const EAGER_LOAD_SIZE = { algolia: 500, npm: 500 } as const | ||
|
|
||
| function paginationTotal(effectiveTotal: number, provider: keyof typeof EAGER_LOAD_SIZE): number { | ||
| const cap = EAGER_LOAD_SIZE[provider] | ||
| return Math.min(effectiveTotal, cap) | ||
| } |
There was a problem hiding this comment.
This test re-implements production logic instead of exercising a shared source of truth.
Lines 12-17 duplicate both EAGER_LOAD_SIZE and the capping function. That can let regressions in app/pages/search.vue pass unnoticed. Prefer extracting the cap logic into a shared util and importing it in both the page and this spec.
Refactor direction (shared util + direct import in tests)
-const EAGER_LOAD_SIZE = { algolia: 500, npm: 500 } as const
-
-function paginationTotal(effectiveTotal: number, provider: keyof typeof EAGER_LOAD_SIZE): number {
- const cap = EAGER_LOAD_SIZE[provider]
- return Math.min(effectiveTotal, cap)
-}
+import { EAGER_LOAD_SIZE, getPaginationTotal } from '~/utils/pagination'- expect(paginationTotal(100, 'npm')).toBe(100)
+ expect(getPaginationTotal(100, 'npm')).toBe(100)(Apply same replacement across the file, and use the same util in app/pages/search.vue.)
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
🔗 Linked issue
resolves #1923
🧭 Context
The search page passed
effectiveTotal(the raw result count from npm/Algolia, which can be 92,000+) directly to the pagination component. This caused the paginator to show thousands of pages, but navigation beyond page 20 (at 25 items per page with a 500-result cap) would silently fail because the API cannot return results past theEAGER_LOAD_SIZElimit.📚 Description
Added a
paginationTotalcomputed property that capseffectiveTotalwithMath.min(effectiveTotal, EAGER_LOAD_SIZE[provider]). The pagination component now receives this capped value, so displayed page counts accurately reflect only the pages that can actually be fetched (max 20 pages at 25 items/page with a 500-item cap).A unit test (
test/unit/app/utils/pagination.spec.ts) verifies the capping logic for values below, at, and above the cap, and checks that the derived page count stays within the fetchable range.