Solution by tavokina · Pull Request #1472 · mate-academy/react_pagination

tavokina · 2026-04-05T11:02:56Z

mateacademy-ai-mentor

Review Summary

Great work on implementing the pagination feature! The component is well-structured and most of the functionality is working correctly. However, a couple of changes are needed in src/components/Pagination/Pagination.tsx to fully align with the task requirements.

First, the currentPage prop is defined as required, but the task specifies it should be optional with a default value of 1. You can fix this by making it optional in the PaginationProps interface (e.g., currentPage?: number) and setting a default value within the component.

Second, the onPageChange callback should only be triggered when the page number actually changes. Currently, it fires even when clicking on the active page. Please add a condition to ensure the callback is only invoked when the selected page is different from currentPage.

Addressing these points will make your solution fully compliant. Keep up the great work!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-04-05T11:05:14Z

 import { getNumbers } from './utils';
+import { Pagination } from './components/Pagination';

 // eslint-disable-next-line @typescript-eslint/no-unused-vars
 const items = getNumbers(1, 42).map(n => `Item ${n}`);



According to the requirements, the currentPage prop should be optional with a default value of 1. Currently, it's defined as a required prop in the PaginationProps interface. You can make it optional in the interface (e.g., currentPage?: number) and then provide a default value within the component.

mateacademy-ai-mentor · 2026-04-05T11:05:14Z

  return (
    <div className="container">
      <h1>Items with Pagination</h1>



The task requires the onPageChange callback to be triggered only if the page was changed. Currently, clicking on the active page link will still call this function. You should add a condition to call onPageChange only when the selected page is different from the currentPage.

mateacademy-ai-mentor · 2026-04-05T11:05:14Z

+
+interface PaginationProps {
+  perPage: number;
+  currentPage: number;


The task description specifies that the currentPage prop should be optional with a default value of 1. Here, it is defined as a required prop. You can make it optional in the PaginationProps interface (e.g., currentPage?: number) and then provide a default value when destructuring the component's props.

mateacademy-ai-mentor · 2026-04-05T11:05:14Z

+        data-cy="pageLink"
+        className="page-link"
+        href={`#${page}`}
+        onClick={() => onPageChange(page)}


The onPageChange callback is triggered even when clicking on the currently active page. The requirements state it should be triggered "only if page was changed". You should add a condition to call onPageChange only when the selected page is different from currentPage.

mateacademy-ai-mentor · 2026-04-05T11:05:14Z

+        data-cy="pageLink"
+        className="page-link"
+        href={`#${page}`}
+        onClick={() => onPageChange(page)}


It's a good practice to prevent the default browser action for <a> tag clicks when handling navigation with JavaScript. This will stop the URL from being updated with the href hash (e.g., #2). You can achieve this by using the event object in your onClick handler and calling event.preventDefault(). This applies to all page links, including the "previous" and "next" buttons.

mateacademy-ai-mentor

Review Summary

Great job implementing the pagination logic and the per-page selector! The core functionality is working well.

However, changes are needed because a key requirement is missing: the pagination state (page and perPage) is not synchronized with the browser's URL. Currently, if you refresh the page, the selected page and items per page are lost.

To fix this, please update the App.tsx component to:

Read the page and perPage values from the URL's query parameters on initial load.
Update the URL whenever the page or the number of items per page changes.

The useSearchParams hook from react-router-dom is the perfect tool for this. Keep up the great work!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-04-05T18:18:41Z

+  currentPage = 1,
+  total,


According to the task description, page and perPage should be stored in the URL. On page load, their values should be read from the URL query parameters. When they change, the URL should be updated. Consider using the useSearchParams hook from react-router-dom to manage this.

mateacademy-ai-mentor · 2026-04-05T18:18:42Z

+  const [elementsPerPage, setElementsPerPage] = useState<PerPageType>(5);
+  const [currentPage, setCurrentPage] = useState(1);


According to the requirements, the current page and items per page should be stored in the URL (e.g., ?page=2&perPage=5). Currently, the component uses local state that isn't synchronized with the URL.

You should use the useSearchParams hook from react-router-dom to:

Read the page and perPage values from the URL when the component first loads to initialize the state.

Update the URL search parameters whenever currentPage or elementsPerPage changes.

mateacademy-ai-mentor

Review Summary

Excellent work implementing the URL synchronization for the pagination state! You've correctly used useSearchParams and addressed the main feedback from the previous review.

I am approving this solution to help you progress, but there's an important improvement to consider for future projects. The component doesn't currently validate the page and perPage values from the URL. This can lead to unexpected behavior if a user enters an out-of-bounds page number (e.g., ?page=99) or an unsupported perPage value. Always validating external inputs makes your application more robust and user-friendly. Also, a small tip: the check for disabling the 'next' button would be more resilient if it used >= instead of == to handle cases where the page number from the URL is already too high.

Overall, you've made fantastic progress. Keep up the great work!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-04-05T19:13:40Z

+}) => {
+  const totalPages = Math.ceil(total / perPage);
+  const pageSelector = getNumbers(1, totalPages).map(page => (
+    <li


The component correctly reads page and perPage from the URL, but it doesn't validate these values. This can lead to a couple of issues:

If the URL has a perPage value that isn't in perPageOptions (e.g., ?perPage=7), the select element won't display it correctly, and the type assertion as PerPageType is unsafe.

If the URL has a page value that is out of bounds (e.g., ?page=99), the component will show an empty list of items.

It's best practice to validate input from external sources like URLs. You should check if perPageFromURL is a valid option and clamp pageFromURL to be within the valid page range (1 to totalPages).

mateacademy-ai-mentor · 2026-04-05T19:13:40Z

+      {pageSelector}
+
+      <li
+        className={`page-item ${currentPage === totalPages ? 'disabled' : ''}`}


This check for disabling the 'next' button should use >= (i.e., currentPage >= totalPages). If currentPage is greater than totalPages (for example, from a URL parameter), this check fails, and the button remains enabled.

The same change is needed for the aria-disabled attribute on line 70 and in the onClick handler on line 73 to make the component robust against out-of-bounds page numbers.

mateacademy-ai-mentor · 2026-04-05T19:13:40Z

+  const pageFromURL = Number(searchParams.get('page')) || 1;
+  const perPageFromURL = Number(searchParams.get('perPage')) || 5;
+  const [currentPage, setCurrentPage] = useState(pageFromURL);
+  const [elementsPerPage, setElementsPerPage] = useState<PerPageType>(perPageFromURL as PerPageType);


It's great that you're reading the page and perPage values from the URL. However, the current implementation doesn't validate these values before using them to initialize the state.

For example:

If the URL has ?perPage=7, elementsPerPage becomes 7. This is not a valid option in the <select> dropdown, which can be confusing for the user. It's better to ensure the perPage value is one of the allowed options.

If the URL has a page number that is out of bounds (e.g., ?page=100 when there are only 9 pages), the component will show an empty list of items and potentially confusing info text.

Consider adding validation for page and perPage from the URL to ensure they fall within a valid range before setting the state.

Solution

e2a84b0

mateacademy-ai-mentor suggested changes Apr 5, 2026

View reviewed changes

Fixed

cb13433

tavokina requested a review from mateacademy-ai-mentor April 5, 2026 18:17

mateacademy-ai-mentor suggested changes Apr 5, 2026

View reviewed changes

URL synch

3bb1b65

tavokina requested a review from mateacademy-ai-mentor April 5, 2026 19:11

mateacademy-ai-mentor approved these changes Apr 5, 2026

View reviewed changes

		const [elementsPerPage, setElementsPerPage] = useState<PerPageType>(5);
		const [currentPage, setCurrentPage] = useState(1);

Conversation

tavokina commented Apr 5, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants