feat(pagination): add getPageUrl prop for SEO-friendly anchor links#1664
feat(pagination): add getPageUrl prop for SEO-friendly anchor links#1664seojcarlos wants to merge 5 commits intothemesberg:mainfrom
Conversation
When getPageUrl is provided, pagination buttons render as <a> elements instead of <button> elements, making pagination links crawlable by search engines. Fixes themesberg#791
|
|
@seojcarlos is attempting to deploy a commit to the Bergside Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds optional getPageUrl(page) to Pagination so controls render as anchors with href when provided; onPageChange is now optional and invoked only if present. Button/navigation components were refactored to support rendering either or . Tests cover link vs button behavior and boundary cases. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Pagination
participant PaginationButton
participant Browser
User->>PaginationButton: click/interact with control
alt getPageUrl provided
PaginationButton->>Browser: navigate to href := getPageUrl(page)
PaginationButton->>Pagination: optionally invoke onPageChange?(page)
Pagination->>Pagination: update currentPage / re-render
else getPageUrl absent
PaginationButton->>Pagination: invoke onPageChange(page)
Pagination->>Pagination: update currentPage / re-render
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/Pagination/Pagination.tsx (1)
47-57:⚠️ Potential issue | 🟠 Major
getPageUrlcannot be used in URL-only mode, and anchor clicks can’t cleanly support client-side handling.
onPageChangeis still mandatory, which conflicts with the requirement to allowgetPageUrlwithout a callback. Also, whenhrefis present, click handlers should optionally prevent default only when a client callback is provided.💡 Proposed fix
export interface BasePaginationProps extends ComponentProps<"nav">, ThemingProps<PaginationTheme> { layout?: "navigation" | "pagination" | "table"; currentPage: number; nextLabel?: string; - onPageChange: (page: number) => void; + onPageChange?: (page: number) => void; previousLabel?: string; showIcons?: boolean; } export interface TablePaginationProps extends BasePaginationProps { layout: "table"; + onPageChange: (page: number) => void; itemsPerPage: number; totalItems: number; } - function goToNextPage() { - onPageChange(nextPage); - } - - function goToPreviousPage() { - onPageChange(previousPage); - } + const handlePageClick = (page: number) => (event: { preventDefault?: () => void }) => { + if (getPageUrl && onPageChange) event.preventDefault?.(); + onPageChange?.(page); + }; <PaginationNavigation className={twMerge(theme.pages.previous.base, showIcon && theme.pages.showIcon)} - onClick={goToPreviousPage} + onClick={handlePageClick(previousPage)} disabled={currentPage === 1} href={getPageUrl && currentPage > 1 ? getPageUrl(previousPage) : undefined} > {renderPaginationButton({ className: twMerge(theme.pages.selector.base, currentPage === page && theme.pages.selector.active), active: page === currentPage, - onClick: () => onPageChange(page), + onClick: handlePageClick(page), href: getPageUrl ? getPageUrl(page) : undefined, children: page, })} <PaginationNavigation className={twMerge(theme.pages.next.base, showIcon && theme.pages.showIcon)} - onClick={goToNextPage} + onClick={handlePageClick(nextPage)} disabled={currentPage === totalPages} href={getPageUrl && currentPage < totalPages ? getPageUrl(nextPage) : undefined} >Also applies to: 116-122, 130-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Pagination/Pagination.tsx` around lines 47 - 57, The Pagination component currently requires onPageChange which prevents pure-URL usage; update types and click logic so getPageUrl/href-only navigation works: make onPageChange optional in BasePaginationProps/DefaultPaginationProps (remove mandatory onPageChange: (page: number) => void) and in the rendering/click handler (where getPageUrl is used) only call preventDefault and invoke onPageChange if onPageChange is provided; if onPageChange is absent allow the anchor to follow its href for normal navigation. Reference the Pagination component, getPageUrl, onPageChange, BasePaginationProps and DefaultPaginationProps when applying these changes.
🧹 Nitpick comments (1)
packages/ui/src/components/Pagination/PaginationButton.tsx (1)
91-99: Remove redundantaria-disabledon the enabled anchor path.That branch only executes when
!disabled, soaria-disabled={disabled}is alwaysfalseand can be dropped for cleaner output.✂️ Proposed cleanup
if (href && !disabled) { return ( <a href={href} className={mergedClassName} onClick={onClick as unknown as ReactEventHandler<HTMLAnchorElement>} - aria-disabled={disabled} {...(props as ComponentProps<"a">)} > {children} </a> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Pagination/PaginationButton.tsx` around lines 91 - 99, In the PaginationButton component, remove the redundant aria-disabled={disabled} prop from the branch that returns an anchor when the condition href && !disabled is true (the enabled anchor path), since disabled is always false there; update the JSX returned in that branch (the <a> element that uses mergedClassName, onClick and spreads props) to omit aria-disabled so the markup is cleaner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/ui/src/components/Pagination/Pagination.tsx`:
- Around line 47-57: The Pagination component currently requires onPageChange
which prevents pure-URL usage; update types and click logic so
getPageUrl/href-only navigation works: make onPageChange optional in
BasePaginationProps/DefaultPaginationProps (remove mandatory onPageChange:
(page: number) => void) and in the rendering/click handler (where getPageUrl is
used) only call preventDefault and invoke onPageChange if onPageChange is
provided; if onPageChange is absent allow the anchor to follow its href for
normal navigation. Reference the Pagination component, getPageUrl, onPageChange,
BasePaginationProps and DefaultPaginationProps when applying these changes.
---
Nitpick comments:
In `@packages/ui/src/components/Pagination/PaginationButton.tsx`:
- Around line 91-99: In the PaginationButton component, remove the redundant
aria-disabled={disabled} prop from the branch that returns an anchor when the
condition href && !disabled is true (the enabled anchor path), since disabled is
always false there; update the JSX returned in that branch (the <a> element that
uses mergedClassName, onClick and spreads props) to omit aria-disabled so the
markup is cleaner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57b043ab-5b26-4a43-9d96-83d7eb157905
📒 Files selected for processing (3)
packages/ui/src/components/Pagination/Pagination.test.tsxpackages/ui/src/components/Pagination/Pagination.tsxpackages/ui/src/components/Pagination/PaginationButton.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/Pagination/PaginationButton.tsx (1)
17-23:⚠️ Potential issue | 🟠 MajorType model is unsound for dual
<button>/<a>rendering.
PaginationButtonProps(line 17) extendsComponentProps<"button">while includinghref?: string(anchor-only prop). The component renders either<a>or<button>at runtime (lines 41–52, 91–102), but the type signature only models button props. This forces unsafe casts at lines 47, 96, and 98 (onClick as unknown as ReactEventHandler<HTMLAnchorElement>,props as ComponentProps<"a">), allowing button-only props to leak onto anchor elements and causing incorrect event handler typing.The suggested discriminated union approach correctly separates button and anchor variants, eliminating casts and preventing invalid prop combinations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Pagination/PaginationButton.tsx` around lines 17 - 23, The prop type is unsound because PaginationButtonProps extends ComponentProps<"button"> while supporting anchor rendering; change to a discriminated union like ButtonProps (extends ComponentProps<"button">, href?: never, type?: ButtonType, onClick?: ReactEventHandler<HTMLButtonElement>) | AnchorProps (extends ComponentProps<"a">, href: string, onClick?: ReactEventHandler<HTMLAnchorElement>) and use the presence of href as the discriminator in the PaginationButton component render logic (the blocks that currently cast onClick and props when returning <a> vs <button>); update the component signature and usages (including any references to onClick, props, and the current casts in the anchor-rendering branches) to use the correct variant types so you can remove the unsafe casts (onClick as unknown as ReactEventHandler<HTMLAnchorElement>, props as ComponentProps<"a">) and get proper typing for both anchor and button elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/ui/src/components/Pagination/PaginationButton.tsx`:
- Around line 17-23: The prop type is unsound because PaginationButtonProps
extends ComponentProps<"button"> while supporting anchor rendering; change to a
discriminated union like ButtonProps (extends ComponentProps<"button">, href?:
never, type?: ButtonType, onClick?: ReactEventHandler<HTMLButtonElement>) |
AnchorProps (extends ComponentProps<"a">, href: string, onClick?:
ReactEventHandler<HTMLAnchorElement>) and use the presence of href as the
discriminator in the PaginationButton component render logic (the blocks that
currently cast onClick and props when returning <a> vs <button>); update the
component signature and usages (including any references to onClick, props, and
the current casts in the anchor-rendering branches) to use the correct variant
types so you can remove the unsafe casts (onClick as unknown as
ReactEventHandler<HTMLAnchorElement>, props as ComponentProps<"a">) and get
proper typing for both anchor and button elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ac22a70-4704-4348-ad13-92ec64958f75
📒 Files selected for processing (1)
packages/ui/src/components/Pagination/PaginationButton.tsx
… variants Replace unsafe type casts with a proper discriminated union pattern. PaginationButtonProps and PaginationPrevButtonProps now use href as the discriminator: when href is provided, anchor-specific props apply; otherwise, button-specific props apply. This removes all 'as unknown as' casts and ensures type safety at compile time. Also removes redundant aria-disabled on enabled anchor elements.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/ui/src/components/Pagination/PaginationButton.tsx (1)
84-92: Consider addingforwardReffor consistency withPaginationButton.
PaginationButtonusesforwardRefbutPaginationNavigationdoes not. If ref access to prev/next controls is needed (e.g., for focus management), this would require addingforwardRef. This is likely pre-existing behavior rather than a regression, so flagging as optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Pagination/PaginationButton.tsx` around lines 84 - 92, PaginationNavigation currently lacks React.forwardRef unlike PaginationButton, so add ref forwarding to enable parent refs (for focus management). Convert the PaginationNavigation function into a React.forwardRef component accepting (props, ref), update its props type to match PaginationPrevButtonProps & Ref attributes (or use React.ComponentPropsWithRef on the rendered element), and attach the forwarded ref to the root DOM element (the same element PaginationButton uses). Keep the existing props (children, className, disabled, theme/customTheme, clearTheme, applyTheme, ...props) and export the forwardRef-wrapped component.packages/ui/src/components/Pagination/Pagination.tsx (2)
130-140: Design note:onPageChangeremains required even withgetPageUrl.When
hrefis provided, clicking the anchor triggers both theonClickhandler (callingonPageChange) and the browser's default navigation. Consumers wanting:
- Pure SEO links (no client-side handling): Must pass a no-op
onPageChange={() => {}}- Client-side routing: Must call
e.preventDefault()in their handlerThis is a valid approach, but consider making
onPageChangeoptional whengetPageUrlis provided to better support the "pure SEO links" use case mentioned in the issue objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Pagination/Pagination.tsx` around lines 130 - 140, The component forces calling onPageChange even when a link URL (getPageUrl / previousHref) is used; update the prop typing so onPageChange is optional and change the click handler logic in PaginationNavigation (and the counterpart for next) to only call onPageChange if it is provided (i.e., guard the call with if (onPageChange) or similar), so pure SEO anchors can be used without requiring a no-op handler; ensure any TypeScript types for onPageChange reflect optionality and adjust any places that assumed it was always present.
146-152: Minor redundancy: active styling applied in two places.The
activeclass is merged intoclassNamehere (line 147) and also applied insidePaginationButtonwhen theactiveprop is true.twMergehandles duplicates gracefully, so there's no visual bug, but it's slightly redundant.This may be intentional to support custom
renderPaginationButtonimplementations that inspect theactiveprop. If so, consider removing the active class from the parent'sclassNameand let the child handle it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Pagination/Pagination.tsx` around lines 146 - 152, The parent is redundantly injecting the active CSS via twMerge into className while also passing active to renderPaginationButton; update the call site around renderPaginationButton so it does not include theme.pages.selector.active in the merged class when page === currentPage (leave only theme.pages.selector.base), and rely on the active prop (passed as active: page === currentPage) for the child component (e.g., PaginationButton) to apply active styling; if custom renderers require the class present, keep the active class but add a comment in the renderPaginationButton call noting why duplication is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/components/Pagination/Pagination.tsx`:
- Around line 130-140: The component forces calling onPageChange even when a
link URL (getPageUrl / previousHref) is used; update the prop typing so
onPageChange is optional and change the click handler logic in
PaginationNavigation (and the counterpart for next) to only call onPageChange if
it is provided (i.e., guard the call with if (onPageChange) or similar), so pure
SEO anchors can be used without requiring a no-op handler; ensure any TypeScript
types for onPageChange reflect optionality and adjust any places that assumed it
was always present.
- Around line 146-152: The parent is redundantly injecting the active CSS via
twMerge into className while also passing active to renderPaginationButton;
update the call site around renderPaginationButton so it does not include
theme.pages.selector.active in the merged class when page === currentPage (leave
only theme.pages.selector.base), and rely on the active prop (passed as active:
page === currentPage) for the child component (e.g., PaginationButton) to apply
active styling; if custom renderers require the class present, keep the active
class but add a comment in the renderPaginationButton call noting why
duplication is intentional.
In `@packages/ui/src/components/Pagination/PaginationButton.tsx`:
- Around line 84-92: PaginationNavigation currently lacks React.forwardRef
unlike PaginationButton, so add ref forwarding to enable parent refs (for focus
management). Convert the PaginationNavigation function into a React.forwardRef
component accepting (props, ref), update its props type to match
PaginationPrevButtonProps & Ref attributes (or use React.ComponentPropsWithRef
on the rendered element), and attach the forwarded ref to the root DOM element
(the same element PaginationButton uses). Keep the existing props (children,
className, disabled, theme/customTheme, clearTheme, applyTheme, ...props) and
export the forwardRef-wrapped component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6de8f1f1-1156-410a-a74f-577fbf45b6fe
📒 Files selected for processing (2)
packages/ui/src/components/Pagination/Pagination.tsxpackages/ui/src/components/Pagination/PaginationButton.tsx
- Make onPageChange optional when getPageUrl is provided, enabling pure SEO anchor navigation without requiring a no-op callback - Add forwardRef to PaginationNavigation for consistency with PaginationButton and to support ref-based focus management - Remove redundant active class from parent className — let the PaginationButton component handle active styling via the active prop
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/Pagination/Pagination.tsx`:
- Around line 51-56: The props currently allow both onPageChange and getPageUrl
to be omitted; change DefaultPaginationProps into a discriminated union so that
either client-side navigation (onPageChange required, getPageUrl omitted/never)
or anchor navigation (getPageUrl required, onPageChange omitted/never) is
enforced. Define two interfaces (e.g., ClientSidePaginationProps and
AnchorPaginationProps) that extend the shared pagination fields, make one
require onPageChange and forbid getPageUrl, the other require getPageUrl and
forbid onPageChange, then set DefaultPaginationProps = ClientSidePaginationProps
| AnchorPaginationProps and update the Pagination component prop type
accordingly so the optional-chaining no-op handlers are no longer reachable.
In `@packages/ui/src/components/Pagination/PaginationButton.tsx`:
- Around line 46-51: Export the missing type aliases used by the public API:
mark PaginationNavigationAsAnchor with "export" and ensure the navigation type
exported from this file matches what index.ts expects by adding an exported type
alias for PaginationNavigation (e.g., export type PaginationNavigation =
PaginationNavigationBaseProps | PaginationNavigationAsButton |
PaginationNavigationAsAnchor or the correct union used by the component). Update
the declarations around PaginationNavigationAsAnchor and
PaginationPrevButtonProps so PaginationNavigationAsAnchor is exported and the
public PaginationNavigation type is exported as a type (not only as a component
constant) to resolve the TypeScript re-export errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d001e099-fd78-4c96-83f3-c23d67a608b4
📒 Files selected for processing (2)
packages/ui/src/components/Pagination/Pagination.tsxpackages/ui/src/components/Pagination/PaginationButton.tsx
- Use discriminated union for DefaultPaginationProps: either onPageChange (client-side) or getPageUrl (anchor-based) must be provided, preventing misconfiguration at compile time - Export PaginationNavigation type alias and PaginationNavigationAsAnchor to fix type re-export from index.ts
1 participant
Summary
Adds a
getPageUrlprop to thePaginationcomponent that renders pagination buttons as<a>elements instead of<button>elements when provided. This improves SEO by making pagination links crawlable by search engines.Fixes #791
Changes
New prop:
getPageUrl?: (page: number) => stringUsage:
When
getPageUrlis provided:<a href="/blog?page=N">instead of<button><a>when navigation is possible<button disabled>— a disabled link has no semantic meaningWhen
getPageUrlis not provided, everything works exactly as before (fully backwards compatible).Why this matters for SEO
Search engines cannot follow
<button>+ JS event handlers to discover paginated content. By rendering real<a>tags withhrefattributes, crawlers can discover and index all pages, and users get benefits like right-click → "Open in new tab", link prefetching, and better accessibility.Files changed
PaginationButton.tsx—PaginationButtonandPaginationNavigationnow accept an optionalhrefprop. When present, they render<a>instead of<button>.Pagination.tsx— AddedgetPageUrlprop toDefaultPaginationProps. Passes computedhrefto buttons and navigation.Pagination.test.tsx— 6 new tests covering anchor rendering, correct hrefs, disabled states, and backwards compatibility.Test results
All 32 tests pass (26 existing + 6 new). TypeScript compilation clean.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests