-
-
Notifications
You must be signed in to change notification settings - Fork 325
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.
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
Changes from all commits
c54a0c0
07a2aa6
69a4f46
d3011a1
acfd4a4
acf7805
f2723f0
0591bc4
9fabdee
4fb2bf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,23 +22,17 @@ const effectiveMode = computed<PaginationMode>(() => | |||||||||||
| shouldShowControls.value ? 'paginated' : 'infinite', | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| // 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) | ||||||||||||
| }) | ||||||||||||
|
|
||||||||||||
|
|
@@ -106,8 +100,8 @@ const visiblePages = computed(() => { | |||||||||||
| function handlePageSizeChange(event: Event) { | ||||||||||||
| const target = event.target as HTMLSelectElement | ||||||||||||
| const value = target.value | ||||||||||||
| // Handle 'all' as a special string value, otherwise parse as number | ||||||||||||
| const newSize = (value === 'all' ? 'all' : Number(value)) as PageSize | ||||||||||||
|
|
||||||||||||
| const newSize = Number(value) as PageSize | ||||||||||||
| pageSize.value = newSize | ||||||||||||
|
Comment on lines
+104
to
105
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: fd -type f -name "PaginationControls.vue" | head -20Repository: npmx-dev/npmx.dev Length of output: 231 🏁 Script executed: fd -type f -path "*shared/types/preferences.ts" | head -5Repository: npmx-dev/npmx.dev Length of output: 231 🏁 Script executed: fd "PaginationControls.vue"Repository: npmx-dev/npmx.dev Length of output: 98 🏁 Script executed: fd "preferences.ts"Repository: npmx-dev/npmx.dev Length of output: 133 🏁 Script executed: cat -n app/components/PaginationControls.vue | sed -n '90,115p'Repository: npmx-dev/npmx.dev Length of output: 831 🏁 Script executed: cat -n shared/types/preferences.tsRepository: npmx-dev/npmx.dev Length of output: 10569 🏁 Script executed: cat -n app/components/PaginationControls.vue | sed -n '111,180p'Repository: npmx-dev/npmx.dev Length of output: 3187 Consider explicit validation of page-size value. Line 104 uses an unchecked cast: Suggested approach- const newSize = Number(value) as PageSize
- pageSize.value = newSize
+ const newSize = Number(value)
+ if (!PAGE_SIZE_OPTIONS.includes(newSize as PageSize)) return
+ pageSize.value = newSize as PageSizeThis follows strict type-safety practices by explicitly validating before state update, rather than trusting the type cast alone. 📝 Committable suggestion
Suggested change
|
||||||||||||
| // Reset to page 1 when changing page size | ||||||||||||
| currentPage.value = 1 | ||||||||||||
|
|
@@ -159,10 +153,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), | ||||||||||||
| })) | ||||||||||||
| " | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a migration/normalisation path for legacy Line 282 removes 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 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".
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainGreat point — let me locate the preferences hydration code first. 🏁 Script executed: Length of output: 71 🏁 Script executed: Length of output: 2548 🏁 Script executed: Length of output: 6924 🏁 Script executed: Length of output: 2556
✏️ Learnings added
🧠 Learnings usedFailed to handle agent chat message. Please try again.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()
},
})
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai you can participate too 🤣
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The concept is solid and self-contained, but there's one Vue-specific concern: mutating reactive state and calling 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 🧠 Learnings used |
||
|
|
||
| // Complete preferences state | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.