Feat: Expand test coverage for Pagination component#178
Feat: Expand test coverage for Pagination component#178
Conversation
WalkthroughThis pull request refactors the pagination component's keyboard navigation and focus management. It converts PaginationLink to a forwardRef component with disabled prop support, updates PaginationContent to filter disabled links when managing focus, rewrites the keydown handler to support ArrowRight, ArrowLeft, Home, and End key navigation, and expands test coverage for keyboard navigation and focus edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 0
🧹 Nitpick comments (4)
src/components/ui/pagination.test.tsx (2)
90-146: Keyboard navigation tests are thorough; consideruserEvent.keyboardas a minor improvementThe ArrowLeft/ArrowRight/Home/End scenarios cover the roving focus behavior well, including wrap‑around and previous/next controls. If you want slightly more realistic interaction simulation in future, you could switch from
fireEvent.keyDownon the navigation container touserEvent.keyboard, but that’s purely optional here.Also applies to: 148-186
188-337: Nice edge‑case coverage for disabled and single‑page scenariosThese tests do a good job validating:
- Single‑page behavior with both prev/next disabled.
- Disabled prev on first page and disabled next on last page.
- Roving focus and Home/End behavior when prev is disabled but next is not.
If you later harden pointer behavior for disabled links (e.g., ignoring clicks), you might add a small test asserting that clicking a disabled control does not change focus or trigger navigation; current tests already cover the keyboard side well.
src/components/ui/pagination.tsx (2)
41-55: Link registration + DOM querying are duplicative and potentially brittle
linksRefis maintained both viaregisterLink/unregisterLinkand via theuseLayoutEffectthat overwrites it withquerySelectorAll('[data-slot="pagination-link"]:not([disabled])'). This dual source of truth makes index bookkeeping (linkIndexvsfocusedIndex) harder to reason about, especially when children are added/removed or re‑ordered.Given that
handleKeyDownalready recomputesfocusableLinksfrom the DOM, consider simplifying by:
- Either relying solely on DOM queries for both
focusLinkand index derivation (e.g.,focusLinkuses a freshquerySelectorAllwith the same selector), and droppingregisterLink/unregisterLink/linkIndex.- Or, if you prefer context registration, stop overwriting
linksRef.currentfrom the DOM and base navigation solely on the registered list.This will make focus management more predictable and reduce the chance of subtle index desynchronization bugs when the children change.
Also applies to: 56-69, 71-79
120-125: Disabled semantics on<a>could be improved for accessibility and click behavior
PaginationLinkintroduces adisabledprop and:
- Filters links with
:not([disabled])to build the focusable list.- Sets
tabIndex={-1}whendisabledis true.- Passes
disabled={disabled}directly to the<a>element.This gives correct behavior for roving tabindex, but:
<a>elements don’t natively supportdisabled; screen readers and browsers won’t treat them as inert buttons.- Pointer clicks on a disabled link will still follow
href/ fireonClickunless you handle that explicitly.If you want disabled pagination controls to behave like true disabled buttons, consider:
- Adding
aria-disabled={disabled || undefined}and possiblyrole="button"when appropriate; and/or- Preventing navigation when disabled (e.g., ignore
onClick/ callevent.preventDefault()whendisabled); or- Rendering a
<button disabled>instead whendisabledis true.Tests already assert the presence of the
disabledattribute andtabindex="-1", so any change should preserve those expectations while improving semantics.Also applies to: 143-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/pagination.test.tsx(6 hunks)src/components/ui/pagination.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/ui/pagination.test.tsx (1)
src/components/ui/pagination.tsx (6)
PaginationLink(220-220)Pagination(218-218)PaginationContent(219-219)PaginationItem(221-221)PaginationPrevious(222-222)PaginationNext(223-223)
src/components/ui/pagination.tsx (2)
src/components/ui/button.tsx (2)
Button(53-53)buttonVariants(53-53)src/lib/utils.ts (1)
cn(9-11)
🔇 Additional comments (3)
src/components/ui/pagination.test.tsx (2)
47-52: Good coverage of initial roving tabindex behaviorAsserting that only the first focusable control (“Go to previous page”) starts with
tabindex="0"and others-1nicely verifies the roving tabindex pattern and aligns the test with the new focus-management behavior.
55-88: Solid regression test for children updatesThis test usefully asserts that when the children set changes, the first focusable link after rerender (
A) regainstabindex="0"while the others remain-1, which guards against regressions infocusedIndexreset logic when pagination items are dynamically updated.src/components/ui/pagination.tsx (1)
126-163: Forwarded ref breaks internal registration when a consumer passesrefIn
PaginationLink, you currently do:const linkRef = React.useRef<HTMLAnchorElement>(null); <a ref={ref || linkRef} ... />and then register based on
linkRef.current. When a consumer supplies a ref,ref || linkRefresolves to the external ref, solinkRef.currentnever gets populated and this link is never registered with thePaginationContext. That means such links won't participate correctly in the roving tabindex / keyboard navigation.Always populate the internal
linkRef, and then fan out the same node to the forwarded ref. For example:-const PaginationLink = React.forwardRef<HTMLAnchorElement, PaginationLinkProps>( - ({ className, isActive, size = "icon", disabled, ...props }, ref) => { +const PaginationLink = React.forwardRef<HTMLAnchorElement, PaginationLinkProps>( + ({ className, isActive, size = "icon", disabled, ...props }, ref) => { const context = React.useContext(PaginationContext); const linkRef = React.useRef<HTMLAnchorElement>(null); @@ - return ( - <a - ref={ref || linkRef} + const mergedRef = React.useCallback( + (node: HTMLAnchorElement | null) => { + linkRef.current = node; + if (typeof ref === "function") { + ref(node); + } else if (ref && "current" in (ref as any)) { + (ref as React.MutableRefObject<HTMLAnchorElement | null>).current = node; + } + }, + [ref] + ); + + return ( + <a + ref={mergedRef} @@ - disabled={disabled} + disabled={disabled} tabIndex={tabIndex} {...props} /> ); } );This keeps your internal registration working regardless of whether a parent supplies a ref, while still honoring the forwarded ref contract.
1 participant
Overview: This PR significantly expands the test coverage for the
Paginationcomponent.Changes
src/components/ui/pagination.test.tsx.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.