Skip to content

Commit 2a15616

Browse files
samuvsamuv
authored
feat(skills): paginate registry tab and preserve state across detail navigation (#2097)
* feat(ui): add reusable Pagination component * feat(skills): paginate registry tab with URL-backed state * fix(skills): preserve pagination state when returning from skill detail * refactor(skills): address PR review feedback - Clamp pageSize to >= 1 in the shared Pagination component so invalid inputs can't produce Infinity totals or negative ranges. - Derive REGISTRY_PAGE_SIZE_OPTIONS / DEFAULT_REGISTRY_PAGE_SIZE from the Pagination component's DEFAULT_PAGE_SIZE_OPTIONS to keep a single source of truth. - Document why the registry skills fixture keeps V01X (uppercase) — MSW's auto-mocker derives the lookup name via its own toPascalCase and would break if aligned with the SDK's V01x casing. Made-with: Cursor
1 parent a76f62e commit 2a15616

10 files changed

Lines changed: 835 additions & 20 deletions

File tree

‎renderer/src/common/components/ui/__tests__/pagination.test.tsx‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import { render, screen } from '@testing-library/react'
2+
import userEvent from '@testing-library/user-event'
3+
import { describe, expect, it, vi } from 'vitest'
4+
import { Pagination } from '../pagination'
5+
6+
function setup(props: Partial<React.ComponentProps<typeof Pagination>> = {}) {
7+
const onPageChange = vi.fn()
8+
const onPageSizeChange = vi.fn()
9+
const utils = render(
10+
<Pagination
11+
page={1}
12+
pageSize={12}
13+
total={100}
14+
onPageChange={onPageChange}
15+
onPageSizeChange={onPageSizeChange}
16+
{...props}
17+
/>
18+
)
19+
return { onPageChange, onPageSizeChange, ...utils }
20+
}
21+
22+
describe('Pagination', () => {
23+
it('renders "Showing X-Y of N" info, current page and controls', () => {
24+
setup({ page: 2, pageSize: 12, total: 100 })
25+
26+
expect(screen.getByText('Showing 13-24 of 100 results')).toBeVisible()
27+
expect(screen.getByText('Page 2')).toBeVisible()
28+
expect(
29+
screen.getByRole('button', { name: /go to first page/i })
30+
).toBeVisible()
31+
expect(
32+
screen.getByRole('button', { name: /go to previous page/i })
33+
).toBeVisible()
34+
expect(
35+
screen.getByRole('button', { name: /go to next page/i })
36+
).toBeVisible()
37+
})
38+
39+
it('uses a custom item label', () => {
40+
setup({ total: 100, itemLabel: 'skills' })
41+
expect(screen.getByText('Showing 1-12 of 100 skills')).toBeVisible()
42+
})
43+
44+
it('disables first and previous on page 1', () => {
45+
setup({ page: 1, pageSize: 12, total: 100 })
46+
47+
expect(
48+
screen.getByRole('button', { name: /go to first page/i })
49+
).toBeDisabled()
50+
expect(
51+
screen.getByRole('button', { name: /go to previous page/i })
52+
).toBeDisabled()
53+
expect(
54+
screen.getByRole('button', { name: /go to next page/i })
55+
).toBeEnabled()
56+
})
57+
58+
it('disables next on the last page', () => {
59+
setup({ page: 9, pageSize: 12, total: 100 })
60+
61+
expect(
62+
screen.getByRole('button', { name: /go to next page/i })
63+
).toBeDisabled()
64+
expect(
65+
screen.getByRole('button', { name: /go to previous page/i })
66+
).toBeEnabled()
67+
})
68+
69+
it('invokes onPageChange when navigation buttons are clicked', async () => {
70+
const user = userEvent.setup()
71+
const { onPageChange } = setup({ page: 3, pageSize: 12, total: 100 })
72+
73+
await user.click(screen.getByRole('button', { name: /go to next page/i }))
74+
expect(onPageChange).toHaveBeenCalledWith(4)
75+
76+
await user.click(
77+
screen.getByRole('button', { name: /go to previous page/i })
78+
)
79+
expect(onPageChange).toHaveBeenCalledWith(2)
80+
81+
await user.click(screen.getByRole('button', { name: /go to first page/i }))
82+
expect(onPageChange).toHaveBeenCalledWith(1)
83+
})
84+
85+
it('invokes onPageSizeChange when a new size is selected', async () => {
86+
const user = userEvent.setup()
87+
const { onPageSizeChange } = setup({ page: 1, pageSize: 12, total: 500 })
88+
89+
await user.click(screen.getByRole('combobox', { name: /items per page/i }))
90+
await user.click(screen.getByRole('option', { name: '50' }))
91+
92+
expect(onPageSizeChange).toHaveBeenCalledWith(50)
93+
})
94+
95+
it('renders nothing when total fits in the smallest page size option', () => {
96+
const { container } = setup({ page: 1, pageSize: 12, total: 5 })
97+
expect(container).toBeEmptyDOMElement()
98+
})
99+
})

‎renderer/src/common/components/ui/pagination.tsx‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
import {
2+
ChevronFirstIcon,
3+
ChevronLeftIcon,
4+
ChevronRightIcon,
5+
} from 'lucide-react'
6+
7+
import { cn } from '@/common/lib/utils'
8+
import { Button } from '@/common/components/ui/button'
9+
import {
10+
Select,
11+
SelectContent,
12+
SelectItem,
13+
SelectTrigger,
14+
SelectValue,
15+
} from '@/common/components/ui/select'
16+
17+
export const DEFAULT_PAGE_SIZE_OPTIONS = [12, 24, 50, 100] as const
18+
19+
type PaginationProps = {
20+
page: number
21+
pageSize: number
22+
total: number
23+
pageSizeOptions?: readonly number[]
24+
onPageChange: (page: number) => void
25+
onPageSizeChange: (size: number) => void
26+
className?: string
27+
itemLabel?: string
28+
}
29+
30+
export function Pagination({
31+
page,
32+
pageSize,
33+
total,
34+
pageSizeOptions = DEFAULT_PAGE_SIZE_OPTIONS,
35+
onPageChange,
36+
onPageSizeChange,
37+
className,
38+
itemLabel = 'results',
39+
}: PaginationProps) {
40+
const smallestOption = Math.min(...pageSizeOptions)
41+
if (total <= 0 || total <= smallestOption) {
42+
return null
43+
}
44+
45+
// Guard against non-positive pageSize so downstream math (ceil / multiply)
46+
// can't produce Infinity or negative ranges.
47+
const safePageSize = Math.max(1, Math.floor(pageSize))
48+
const safePage = Math.max(1, page)
49+
const totalPages = Math.max(1, Math.ceil(total / safePageSize))
50+
const currentPage = Math.min(safePage, totalPages)
51+
const firstItem = total === 0 ? 0 : (currentPage - 1) * safePageSize + 1
52+
const lastItem = Math.min(currentPage * safePageSize, total)
53+
const isFirstPage = currentPage <= 1
54+
const isLastPage = currentPage >= totalPages
55+
56+
return (
57+
<div
58+
className={cn(
59+
`bg-background flex h-[52px] items-center justify-between rounded-md
60+
border py-2 pr-2 pl-4`,
61+
className
62+
)}
63+
data-slot="pagination"
64+
>
65+
<p className="text-muted-foreground text-sm">
66+
Showing {firstItem}-{lastItem} of {total} {itemLabel}
67+
</p>
68+
<div className="flex items-center justify-center gap-2 opacity-80">
69+
<Button
70+
variant="ghost"
71+
size="icon"
72+
aria-label="Go to first page"
73+
disabled={isFirstPage}
74+
onClick={() => onPageChange(1)}
75+
>
76+
<ChevronFirstIcon />
77+
</Button>
78+
<Button
79+
variant="ghost"
80+
size="icon"
81+
aria-label="Go to previous page"
82+
disabled={isFirstPage}
83+
onClick={() => onPageChange(currentPage - 1)}
84+
>
85+
<ChevronLeftIcon />
86+
</Button>
87+
<p
88+
className="text-foreground px-1 text-sm whitespace-nowrap"
89+
aria-live="polite"
90+
>
91+
Page {currentPage}
92+
</p>
93+
<Button
94+
variant="ghost"
95+
size="icon"
96+
aria-label="Go to next page"
97+
disabled={isLastPage}
98+
onClick={() => onPageChange(currentPage + 1)}
99+
>
100+
<ChevronRightIcon />
101+
</Button>
102+
</div>
103+
<div className="flex items-center gap-2">
104+
<p className="text-secondary-foreground text-sm whitespace-nowrap">
105+
Items per page
106+
</p>
107+
<Select
108+
value={String(pageSize)}
109+
onValueChange={(value) => onPageSizeChange(Number(value))}
110+
>
111+
<SelectTrigger
112+
aria-label="Items per page"
113+
className="h-10 border-0 shadow-none"
114+
>
115+
<SelectValue />
116+
</SelectTrigger>
117+
<SelectContent align="end">
118+
{pageSizeOptions.map((option) => (
119+
<SelectItem key={option} value={String(option)}>
120+
{option}
121+
</SelectItem>
122+
))}
123+
</SelectContent>
124+
</Select>
125+
</div>
126+
</div>
127+
)
128+
}

‎renderer/src/common/mocks/fixtures/registry_registryName_v0_1_x_dev_toolhive_skills/get.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import type {
2+
GetRegistryByRegistryNameV01xDevToolhiveSkillsResponse,
3+
GetRegistryByRegistryNameV01xDevToolhiveSkillsData,
4+
} from '@common/api/generated/types.gen'
5+
import { AutoAPIMock } from '@mocks'
6+
7+
// Name intentionally uses `V01X` (uppercase X): it must match the mock name
8+
// derived by `mocker.ts#toPascalCase` from the path segment `v0.1.x`, which is
9+
// what MSW looks up at runtime. The generated SDK types use `V01x` because
10+
// they come from a different codegen (`@hey-api/openapi-ts`) with its own
11+
// casing rules — don't align these without also updating the mocker.
12+
export const mockedGetRegistryByRegistryNameV01XDevToolhiveSkills = AutoAPIMock<
13+
GetRegistryByRegistryNameV01xDevToolhiveSkillsResponse,
14+
GetRegistryByRegistryNameV01xDevToolhiveSkillsData
15+
>({
16+
skills: [
17+
{
18+
name: 'sample-skill',
19+
namespace: 'io.github.sample',
20+
description: 'A sample registry skill used as fixture default.',
21+
},
22+
],
23+
metadata: {
24+
page: 1,
25+
limit: 12,
26+
total: 1,
27+
},
28+
})

‎renderer/src/features/registry-servers/components/registry-detail-header.tsx‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,22 @@
11
import { Button } from '@/common/components/ui/button'
22
import { LinkViewTransition } from '@/common/components/link-view-transition'
33
import { ChevronLeft } from 'lucide-react'
4-
import type { ReactNode } from 'react'
4+
import type { MouseEvent, ReactNode } from 'react'
5+
import { useCanGoBack, useRouter } from '@tanstack/react-router'
56

67
type RegistryDetailHeaderProps = {
78
title: string
89
backTo?: string
910
backSearch?: Record<string, unknown>
1011
badges?: ReactNode
1112
description?: string | null
13+
/**
14+
* When true, clicking the back button navigates using browser history
15+
* (`router.history.back()`) so the previous search params (e.g. pagination)
16+
* are restored. Falls back to navigating to `backTo` when there is no
17+
* in-app history to go back to (direct deep-links).
18+
*/
19+
historyBack?: boolean
1220
}
1321

1422
export function RegistryDetailHeader({
@@ -17,11 +25,36 @@ export function RegistryDetailHeader({
1725
backSearch,
1826
badges,
1927
description,
28+
historyBack = false,
2029
}: RegistryDetailHeaderProps) {
30+
const router = useRouter()
31+
const canGoBack = useCanGoBack()
32+
33+
const handleBackClick = (event: MouseEvent<HTMLAnchorElement>) => {
34+
if (!historyBack || !canGoBack) return
35+
// Let the browser handle modifier-clicks (open in new tab, etc.)
36+
if (
37+
event.defaultPrevented ||
38+
event.button !== 0 ||
39+
event.metaKey ||
40+
event.ctrlKey ||
41+
event.shiftKey ||
42+
event.altKey
43+
) {
44+
return
45+
}
46+
event.preventDefault()
47+
router.history.back()
48+
}
49+
2150
return (
2251
<div className="w-full">
2352
<div className="mb-5">
24-
<LinkViewTransition to={backTo} search={backSearch}>
53+
<LinkViewTransition
54+
to={backTo}
55+
search={backSearch}
56+
onClick={handleBackClick}
57+
>
2558
<Button variant="outline" aria-label="Back" className="rounded-full">
2659
<ChevronLeft className="size-4" />
2760
Back

0 commit comments

Comments
 (0)