Skip to content

fix(ui): replace "yolo" mode with npm API max of 250 results #1922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
serhalp merged 10 commits into from
Mar 5, 2026
Merged

fix(ui): replace "yolo" mode with npm API max of 250 results #1922

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c54a0c0
fix(ui): yolo shows incorrect results
MatteoGabriele Mar 4, 2026
07a2aa6
refactor: remove instances of "all"
MatteoGabriele Mar 4, 2026
69a4f46
Merge branch 'main' into fix/search-yolo-pagination-
MatteoGabriele Mar 4, 2026
d3011a1
chore: remove stale comment
MatteoGabriele Mar 4, 2026
acfd4a4
Merge branch 'fix/search-yolo-pagination-' of https://github.com/Matt…
MatteoGabriele Mar 4, 2026
acf7805
fix: i18n unused keys
MatteoGabriele Mar 4, 2026
f2723f0
refactor: remove "all" logic
MatteoGabriele Mar 4, 2026
0591bc4
fix: migration of "all" in localStorage
MatteoGabriele Mar 5, 2026
9fabdee
refactor: remove checks
MatteoGabriele Mar 5, 2026
4fb2bf2
Merge branch 'main' into fix/search-yolo-pagination-
MatteoGabriele Mar 5, 2026
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
12 changes: 2 additions & 10 deletions app/components/PaginationControls.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,17 @@ const effectiveMode = computed<PaginationMode>(() =>
)

// When 'all' is selected, there's only 1 page with everything

@maxchang3 maxchang3 Mar 4, 2026

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.

Suggested change
// When 'all' is selected, there's only 1 page with everything

const isShowingAll = computed(() => pageSize.value === 'all')
const totalPages = computed(() =>
isShowingAll.value ? 1 : Math.ceil(props.totalItems / (pageSize.value as number)),
)
const totalPages = computed(() => Math.ceil(props.totalItems / (pageSize.value as number)))

// Whether to show the mode toggle (hidden in table view since table always uses pagination)
const showModeToggle = computed(() => props.viewMode !== 'table')

const startItem = computed(() => {
if (props.totalItems === 0) return 0
if (isShowingAll.value) return 1
return (currentPage.value - 1) * (pageSize.value as number) + 1
})

const endItem = computed(() => {
if (isShowingAll.value) return props.totalItems
return Math.min(currentPage.value * (pageSize.value as number), props.totalItems)
})

Expand Down Expand Up @@ -159,10 +154,7 @@ function handlePageSizeChange(event: Event) {
@change="handlePageSizeChange"
:items="
PAGE_SIZE_OPTIONS.map(size => ({
label:
size === 'all'
? $t('filters.pagination.all_yolo')
: $t('filters.pagination.per_page', { count: size }),
label: $t('filters.pagination.per_page', { count: size }),
value: String(size),
}))
"
Expand Down
2 changes: 1 addition & 1 deletion shared/types/preferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ export const DEFAULT_FILTERS: StructuredFilters = {
// Pagination modes
export type PaginationMode = 'infinite' | 'paginated'

export const PAGE_SIZE_OPTIONS = [10, 25, 50, 100, 'all'] as const
export const PAGE_SIZE_OPTIONS = [10, 25, 50, 100, 250] as const
export type PageSize = (typeof PAGE_SIZE_OPTIONS)[number]
Comment on lines +282 to 283

@coderabbitai coderabbitai Bot Mar 4, 2026 •
edited
Loading

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

Add a migration/normalisation path for legacy 'all' page sizes.

Line 282 removes 'all' from PageSize, but persisted preferences can still hydrate 'all' from storage. That stale value can flow into pagination arithmetic and yield NaN ranges/pages for returning users.

Proposed patch in this file 
 export const PAGE_SIZE_OPTIONS = [10, 25, 50, 100, 250] as const
 export type PageSize = (typeof PAGE_SIZE_OPTIONS)[number]
+
+export function isPageSize(value: unknown): value is PageSize {
+  return typeof value === 'number' && PAGE_SIZE_OPTIONS.includes(value as PageSize)
+}
+
+export function normalisePageSize(value: unknown, fallback: PageSize = 25): PageSize {
+  return isPageSize(value) ? value : fallback
+}

Then use normalisePageSize(stored.pageSize) when hydrating preferences in app/composables/usePreferencesProvider.ts.

As per coding guidelines, "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".

@serhalp serhalp Mar 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

^ this is actually a good point 🐰. We persist preferences to localStorage. We should add logic where we read this from local storage to default it to our default (250).

@coderabbitai coderabbitai Bot Mar 5, 2026

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.

🧩 Analysis chain

Great point — let me locate the preferences hydration code first.

🏁 Script executed:

#!/bin/bash
# Find the preferences provider composable
fd -t f 'usePreferences' --exec echo {}

# Also search for localStorage usage related to preferences/pageSize
rg -n "pageSize|PAGE_SIZE|localStorage|useStorage|hydrat" --type vue --type ts -C 2 -g "!node_modules" -g "!dist"

Length of output: 71

🏁 Script executed:

#!/bin/bash
# Read the preferences provider composable
cat app/composables/usePreferencesProvider.ts

# Search for pageSize/localStorage/useStorage in ts files
rg -n "pageSize\|PAGE_SIZE\|localStorage\|useStorage\|normalise\|hydrat" --type ts -C 3 -g "!node_modules"

# Search in .vue files separately
rg -n "pageSize\|PAGE_SIZE\|localStorage\|useStorage\|normalise\|hydrat" -C 3 -g "*.vue" -g "!node_modules"

Length of output: 2548

🏁 Script executed:

#!/bin/bash
# Find where usePreferencesProvider is consumed
rg -n "usePreferencesProvider\|PackageListPreferences\|DEFAULT_PREFERENCES" --type ts -C 3 -g "!node_modules"

# Also find any composable that references pageSize + preferences together
rg -n "pageSize" --type ts -C 3 -g "!node_modules"

Length of output: 6924

🏁 Script executed:

#!/bin/bash
cat app/composables/usePackageListPreferences.ts

Length of output: 2556

@serhalp — here's the full picture from the code and the plan:

  • usePreferencesProvider.ts uses defu(stored, defaultValue) on mount. defu only fills in undefined/null keys, so a stored 'all' survives the merge unchanged and flows into pagination arithmetic.

  • usePackageListPreferences.ts is the right place to apply the fix — it owns both the hydration flag (isHydrated) and the save() call, so we can normalise the stale value and immediately re-persist the corrected value to localStorage in one step.

  • Add isPageSize + normalisePageSize guards to shared/types/preferences.ts

  • Watch isHydrated in usePackageListPreferences.ts to normalise 'all' → 250 on first hydration and re-persist

✏️ Learnings added 
Learnt from: serhalp
Repo: npmx-dev/npmx.dev PR: 1922
File: shared/types/preferences.ts:282-283
Timestamp: 2026-03-05T00:49:15.091Z
Learning: In npmx-dev/npmx.dev, the preferences hydration in app/composables/usePreferencesProvider.ts uses defu(stored, defaultValue) on onMounted. defu only fills in null/undefined keys, so a stale persisted value (e.g. legacy 'all' pageSize) survives the merge unchanged. Normalisation/migration of stale stored values must be done explicitly after hydration, not via defu defaults.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: MatteoGabriele
Repo: npmx-dev/npmx.dev PR: 1922
File: app/pages/org/[org].vue:68-69
Timestamp: 2026-03-04T13:25:19.779Z
Learning: In npmx-dev/npmx.dev, PAGE_SIZE_OPTIONS in shared/types/preferences.ts contains only strictly numeric values: 10, 25, 50, 100, and 250. The previous 'all' string option was removed in PR `#1922`. The derived PageSize type is therefore 10 | 25 | 50 | 100 | 250 — no non-numeric values exist. Code that reads pageSize.value (e.g., in app/pages/org/[org].vue) can safely use ?? 25 without needing Number() coercion, as TypeScript enforces numeric-only values.

Failed to handle agent chat message. Please try again.

@MatteoGabriele MatteoGabriele Mar 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@serhalp, what if I drop something like this into the usePackageListPreferences file instead of checking for "all"?

const pageSize = computed({
    get: () => {
      const size = preferences.value.pageSize as unknown

      // Migrate legacy "all" value to the highest number
      if (size === 'all') {
        const max = Math.max(...PAGE_SIZE_OPTIONS) as PageSize
        preferences.value.pageSize = max
        save()
        return max
      }
      return preferences.value.pageSize
    },
    set: (value: PageSize) => {
      preferences.value.pageSize = value
      save()
    },
  })

@coderabbitai coderabbitai Bot Mar 5, 2026

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.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@MatteoGabriele MatteoGabriele Mar 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@coderabbitai you can participate too 🤣

@coderabbitai coderabbitai Bot Mar 5, 2026

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.

@MatteoGabriele — happy to join! 🐰

The concept is solid and self-contained, but there's one Vue-specific concern: mutating reactive state and calling save() inside a computed getter is a side effect, which Vue discourages. The reactivity system can invoke getters multiple times during a render cycle, and while in practice preferences.value.pageSize would flip to a numeric value on the first invocation (preventing repeated save() calls), Vue may log a warning.

A small refinement that keeps the same inline spirit but moves the side effect out of the getter:

// One-time migration: replace legacy 'all' with the current maximum page size
watch(isHydrated, (hydrated) => {
  if (!hydrated) return
  if ((preferences.value.pageSize as unknown) === 'all') {
    preferences.value.pageSize = Math.max(...PAGE_SIZE_OPTIONS) as PageSize
    save()
  }
}, { immediate: true })

const pageSize = computed({
  get: () => preferences.value.pageSize,
  set: (value: PageSize) => {
    preferences.value.pageSize = value
    save()
  },
})

This runs exactly once after hydration, keeps the getter pure, and achieves the same outcome — stale 'all' gets replaced with 250 and persisted back to localStorage. The Math.max(...PAGE_SIZE_OPTIONS) trick from your snippet is a nice touch to avoid hardcoding 250.

🧠 Learnings used 
Learnt from: MatteoGabriele
Repo: npmx-dev/npmx.dev PR: 1922
File: app/pages/org/[org].vue:68-69
Timestamp: 2026-03-04T13:25:19.779Z
Learning: In npmx-dev/npmx.dev, PAGE_SIZE_OPTIONS in shared/types/preferences.ts contains only strictly numeric values: 10, 25, 50, 100, and 250. The previous 'all' string option was removed in PR `#1922`. The derived PageSize type is therefore 10 | 25 | 50 | 100 | 250 — no non-numeric values exist. Code that reads pageSize.value (e.g., in app/pages/org/[org].vue) can safely use ?? 25 without needing Number() coercion, as TypeScript enforces numeric-only values.

Learnt from: serhalp
Repo: npmx-dev/npmx.dev PR: 1922
File: shared/types/preferences.ts:282-283
Timestamp: 2026-03-05T00:49:15.091Z
Learning: In npmx-dev/npmx.dev, the preferences hydration in app/composables/usePreferencesProvider.ts uses defu(stored, defaultValue) on onMounted. defu only fills in null/undefined keys, so a stale persisted value (e.g. legacy 'all' pageSize) survives the merge unchanged. Normalisation/migration of stale stored values must be done explicitly after hydration, not via defu defaults.


// Complete preferences state
Expand Down
Loading