Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion app/pages/search.vue
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,18 @@ const effectiveTotal = computed(() => {
return displayResults.value.length
})

/**
* Total items for pagination purposes.
* Capped by EAGER_LOAD_SIZE so that the page count only reflects pages we can
* actually fetch — e.g. with a 500-result cap, max pages = ceil(500 / pageSize).
* Without this cap, a search returning total=92,000 would show 3,680 pages but
* navigation beyond page 20 (at 25/page) would silently fail.
*/
const paginationTotal = computed(() => {
const cap = EAGER_LOAD_SIZE[searchProvider.value]
return Math.min(effectiveTotal.value, cap)
})
Comment on lines +275 to +278
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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.vue

Repository: 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.

Suggested change
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)
}
})


// Handle filter chip removal
function handleClearFilter(chip: FilterChip) {
clearFilter(chip)
Expand Down Expand Up @@ -878,7 +890,7 @@ onBeforeUnmount(() => {
v-model:mode="paginationMode"
v-model:page-size="preferredPageSize"
v-model:current-page="currentPage"
:total-items="effectiveTotal"
:total-items="paginationTotal"
:view-mode="viewMode"
/>
</div>
Expand Down
52 changes: 52 additions & 0 deletions test/unit/app/utils/pagination.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { describe, expect, it } from 'vitest'

/**
* Tests for the pagination total capping logic used in search.vue.
*
* The search page caps the displayed pagination total to EAGER_LOAD_SIZE
* so that page links only reflect pages that can actually be fetched.
* Without the cap, a search returning total=92,000 items would show 3,680
* pages at 25 items/page, but navigation beyond page 20 silently fails.
*/

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)
}
Comment on lines +12 to +17
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.)


describe('paginationTotal capping logic', () => {
it('returns the total as-is when it is below the cap', () => {
expect(paginationTotal(100, 'npm')).toBe(100)
expect(paginationTotal(100, 'algolia')).toBe(100)
})

it('returns the cap when the total exceeds it', () => {
expect(paginationTotal(92_000, 'npm')).toBe(500)
expect(paginationTotal(92_000, 'algolia')).toBe(500)
})

it('returns exactly the cap when the total equals the cap', () => {
expect(paginationTotal(500, 'npm')).toBe(500)
expect(paginationTotal(500, 'algolia')).toBe(500)
})

it('returns 0 when total is 0', () => {
expect(paginationTotal(0, 'npm')).toBe(0)
})

it('caps algolia and npm identically (both have 500 limit)', () => {
const total = 10_000
expect(paginationTotal(total, 'algolia')).toBe(paginationTotal(total, 'npm'))
})

it('page count derived from capped total stays within fetchable range', () => {
const pageSize = 25
const rawTotal = 92_000
const cappedTotal = paginationTotal(rawTotal, 'npm')
const maxPages = Math.ceil(cappedTotal / pageSize)
// Should be 20 pages (500 / 25), not 3680 (92000 / 25)
expect(maxPages).toBe(20)
})
})
Loading