paginator by masteryada378 · Pull Request #1521 · mate-academy/react_pagination

masteryada378 · 2026-06-26T05:24:06Z

mateacademy-ai-mentor

Review Summary

I am requesting changes because a critical functional requirement is missing: the pagination state is not synced with the URL via React Router. According to the task, page and perPage must be stored in the query string (e.g. ?page=2&perPage=7) and used to initialize the app state on page load ([CHECKLIST ITEM #13] and [CHECKLIST ITEM #15]), but currently currentPage and perPage are held only in local state in src/App.tsx and src/components/Pagination/Pagination.tsx and never read from or written to the URL. This means reloading the page or sharing a link does not preserve pagination settings, which breaks a core part of the required behavior.

The main change you need is to integrate React Router and its search-param APIs: for example, wrap your app in BrowserRouter, then in App.tsx use hooks like useSearchParams (or useLocation + useNavigate) to:

Read page and perPage from the query string on initial render (with safe defaults like 1 and 5), and initialize your state from those values.
Whenever currentPage or perPage changes (including via onPageChange or the per-page <select>), update the URL search params to keep it in sync.
You can keep Pagination itself stateless and controlled by props, but its parent must derive and persist those props via the router. Once this routing integration is added and tests pass with the expected URL behavior, the rest of your implementation (page calculation, disabled states, info text, per-page selector) looks solid and can be accepted.

✨ 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-06-26T05:26:28Z

+  const [currentPage, setCurrentPage] = useState<number>(1);
+  const [perPage, setPerPage] = useState<number>(5);


Checklist items #13 and #15 require using React Router to store ?page=...&perPage=... in the URL and to apply those values on page load. This component currently keeps currentPage and perPage only in local state and never reads/writes them from/to the URL, so the router-related requirement is missing.

mateacademy-ai-mentor · 2026-06-26T05:26:28Z

+  const handlePageChange = (page: number) => {
+    setCurrentPage(page);
+  };


For the pagination task, you also need to initialize currentPage (and perPage) from URL query params via React Router and update the query string when the page changes, so that ?page=2&perPage=7 is reflected in and driven by the URL as required.

mateacademy-ai-mentor · 2026-06-26T05:26:28Z

+}
+


The task requires using React Router to save ?page=2&perPage=7 in the URL and apply them on page load (checklist items #13 and #15), but this component currently manages currentPage and perPage only in local state without any router integration. Consider initializing these values from the URL query parameters and updating the URL whenever they change.

mateacademy-ai-mentor · 2026-06-26T05:26:28Z

+    event: React.MouseEvent<HTMLAnchorElement>,
+    page: number,
+  ) => {


Although the pagination itself is implemented correctly, the requirement to persist page and perPage in the URL via React Router is missing. You’ll need to integrate router hooks here (e.g. read search params on mount and update them when currentPage or perPage changes) so the app state matches the URL and can be restored on reload.

mateacademy-ai-mentor

Review Summary

I am requesting changes because one explicit technical requirement is still not met: React Router is not used to manage the page and perPage query parameters. Right now, in src/App.tsx and src/components/Pagination/Pagination.tsx, you read from window.location.search and update the URL with window.history.pushState, which bypasses React Router and violates the requirement to sync pagination state via router APIs. To fix this, you should wrap the app in BrowserRouter (typically in index.tsx) and, in App.tsx, replace the manual window.location / pushState logic with hooks like useSearchParams (or useLocation + useNavigate) so ?page=...&perPage=... are read and updated through React Router. For example, use const [searchParams, setSearchParams] = useSearchParams(); to read page/perPage, and call setSearchParams({ page: String(newPage), perPage: String(newPerPage) }) inside your handlers instead of using the History API directly. A smaller improvement you might also consider is clamping page to the valid range when reading from the URL (e.g. preventing ?page=999 from resulting in an empty list after a perPage change). Aside from the router integration, your pagination component and UI behavior are very solid, so once you refactor the URL handling to use React Router, your solution will fully meet the task requirements.

✨ 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-06-26T05:56:03Z


 export const App: React.FC = () => {
+  const getParamsFromUrl = () => {
+    const params = new URLSearchParams(window.location.search);


The requirements explicitly say to use React Router to keep page and perPage in the URL; here you are manually reading from window.location.search instead of using router search-param APIs like useSearchParams, so this doesn’t yet meet that technical constraint.

mateacademy-ai-mentor · 2026-06-26T05:56:03Z

+
+    const newUrl = `${window.location.pathname}?${params.toString()}`;
+
+    window.history.pushState({}, '', newUrl);


Similarly, this manual pushState updates the URL without React Router; the task expects pagination state to be synced via React Router (e.g. useSearchParams or useNavigate), so you should refactor this to use the router instead of the global history API.

mateacademy-ai-mentor · 2026-06-26T05:56:03Z

+  currentPage?: number;
+  onPageChange: (page: number) => void;
+}
+


The requirements explicitly state using React Router to persist page and perPage in the URL and apply them on load; here you read from window.location.search directly rather than using React Router hooks like useSearchParams or useLocation, so the technical constraint about router integration is not fulfilled.

mateacademy-ai-mentor · 2026-06-26T05:56:03Z

+
+  return (
+    <ul className="pagination">
+      <li className={`page-item ${isFirstPage ? 'disabled' : ''}`}>


Updating the URL via window.history.pushState works functionally, but the task expects URL synchronization to be done through React Router APIs (e.g. useSearchParams + setSearchParams), so this manual history manipulation should be refactored to use the router.

mateacademy-ai-mentor

Review Summary

I am approving your solution to help you progress educationally while still highlighting a couple of things to double‑check. You’ve implemented a clean, stateless Pagination component that correctly uses total, perPage, and an optional currentPage (defaulting to 1), and you handle page changes, previous/next navigation, and the info text (Page X (items A - B of N)) exactly as required. It’s also great that you now use React Router’s useSearchParams to read and write ?page=...&perPage=... so pagination state is synced with the URL and applied on page load.

One thing to verify against the tests is that all interactive elements have the exact data-cy attributes Cypress expects: page number links as data-cy="pageLink", the per-page <select> as data-cy="perPageSelector", and the previous/next controls as data-cy="prevLink" and data-cy="nextLink"; even a small naming or structural mismatch can cause all E2E tests to fail. If any of those attributes are missing or on a different element than the tests assume, move or add them without changing the core behavior of your component. Overall, your logic, URL integration, and adherence to the pagination requirements are strong—tightening up those test hooks will make the solution fully robust. Keep going; you’re very close to a production‑ready implementation and this is solid progress.

✨ 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.

brespect

Good progress, but your DEMO LINK is not working

masteryada378 · 2026-06-26T12:03:10Z

Sorry, forgot to deploy

Denys-Kravchuk9988

Good job!

A few things to improve:

Denys-Kravchuk9988 · 2026-06-26T16:50:57Z

+
+  return (
+    <ul className="pagination">
+      <li className={`page-item ${isFirstPage ? 'disabled' : ''}`}>


It's better to use classNames here

Denys-Kravchuk9988 · 2026-06-26T16:51:02Z

+      {pages.map(page => (
+        <li
+          key={page}
+          className={`page-item ${currentPage === page ? 'active' : ''}`}


It's better to use classNames here

Denys-Kravchuk9988 · 2026-06-26T16:51:06Z

+        </li>
+      ))}
+
+      <li className={`page-item ${isLastPage ? 'disabled' : ''}`}>


It's better to use classNames here

vadiimvooo

Well done

paginator

39c3d79

mateacademy-ai-mentor suggested changes Jun 26, 2026

View reviewed changes

upd

80b74f6

masteryada378 requested a review from mateacademy-ai-mentor June 26, 2026 05:55

mateacademy-ai-mentor suggested changes Jun 26, 2026

View reviewed changes

2upd

587f319

masteryada378 requested a review from mateacademy-ai-mentor June 26, 2026 06:03

mateacademy-ai-mentor suggested changes Jun 26, 2026

View reviewed changes

2.1upd

66bd539

masteryada378 requested a review from mateacademy-ai-mentor June 26, 2026 07:55

masteryada378 added 2 commits June 26, 2026 11:25

2.2 upd

084b54f

2.21 upd

a100682

brespect suggested changes Jun 26, 2026

View reviewed changes

masteryada378 requested a review from brespect June 26, 2026 12:03

Denys-Kravchuk9988 suggested changes Jun 26, 2026

View reviewed changes

add classNames

6513e8b

masteryada378 requested a review from Denys-Kravchuk9988 June 27, 2026 18:42

vadiimvooo approved these changes Jun 27, 2026

View reviewed changes

		const [currentPage, setCurrentPage] = useState<number>(1);
		const [perPage, setPerPage] = useState<number>(5);


		const newUrl = `${window.location.pathname}?${params.toString()}`;

		window.history.pushState({}, '', newUrl);

Uh oh!

Conversation

masteryada378 commented Jun 26, 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!

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!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

brespect left a comment

Choose a reason for hiding this comment

Uh oh!

masteryada378 commented Jun 26, 2026

Uh oh!

Denys-Kravchuk9988 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadiimvooo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants