Dev by 0-nira-0 · Pull Request #1434 · mate-academy/react_pagination

0-nira-0 · 2026-02-11T19:16:09Z

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still critical functional issues in the pagination behavior and one required feature is missing. In src/components/Pagination/Pagination.tsx, the prev/next handlers currently call onPageChange(currentPage - 1) and onPageChange(currentPage + 1) even when on the first or last page, which can produce invalid page numbers and violates the requirement that callbacks only fire when the page actually changes ([CHECKLIST ITEM #5]). You should guard these handlers and the numbered page links so they only call onPageChange when the target page is within 1..numberOfPage and different from currentPage, and also event.preventDefault() on the anchor clicks (especially when disabled) to avoid unwanted navigation and keep accessibility semantics consistent ([CHECKLIST ITEMS #7, #8, #9]). Also in Pagination.tsx, the page link href is currently written as the literal string "#{i}"; change this to either href={"#"} plus preventDefault() or to a proper template like href={#${i}} so the markup is valid ([CHECKLIST ITEM #3]).

In src/App.tsx, the per-page <select data-cy="perPageSelector"> is uncontrolled and uses selected on an <option>, which is not the React way to control selection; instead, bind value={perPage} on the <select> and remove any selected attributes so the control remains in sync with React state ([CHECKLIST ITEM #11]). Additionally, the task requires syncing page and perPage with the URL query (e.g. ?page=2&perPage=7) using React Router: on initial mount, read these values from location.search to set your initial state, and whenever page or perPage change, update the query parameters so reloading the page restores the current view ([CHECKLIST ITEM #13]). Once you fix the click guards/validation in Pagination, make the per-page select controlled, and implement the URL sync, your solution should fully satisfy the functional requirements; the rest of your implementation (info text, slicing items, stateless Pagination, disabled/active classes) is on the right track. Keep going—you’re very close, and these changes will make your pagination both robust and aligned with React best practices.

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

src/App.tsx

mateacademy-ai-mentor · 2026-02-11T19:21:23Z

src/App.tsx

+  TWENTY = 20,
+}

 export const App: React.FC = () => {


The app currently doesn't persist page and perPage in the URL or read them on load. The task's checklist includes using React Router to save/apply ?page=2&perPage=7 in the URL (checklist item #13). Add URL-sync (read from query on mount and update query params when page/perPage change) so the app restores state from the URL on page load.

src/components/Pagination/Pagination.tsx

mateacademy-ai-mentor

Review Summary

I am requesting changes because a critical requirement about URL persistence is still missing and this blocks full task completion. In src/App.tsx, the app does not yet read and write the page and perPage query parameters via React Router, so reloading the page does not restore the current view as required (e.g. ?page=2&perPage=7) [CHECKLIST ITEM #13]. You need to: 1) initialize currentPage and perPage from location.search / useSearchParams on mount (with sensible fallbacks like page=1, perPage=5), and 2) update the URL query string whenever currentPage or perPage change, either directly in the handlers or in a useEffect that watches those state values. Additionally, in App.tsx, each <option> inside the controlled <select data-cy="perPageSelector"> should have an explicit numeric value attribute (e.g. <option value={3}>3</option>) to avoid type mismatches and subtle controlled-component bugs. In src/components/Pagination/Pagination.tsx, the numbered page links should attach the onClick handler to the <a> element itself and call event.preventDefault() there, matching the behavior you already implemented for the prev/next links and preventing unwanted navigation to the href anchor. Once you add the URL sync logic and adjust these event-handling and option-value details, your solution will align well with the functional and accessibility requirements; your existing pagination logic and state handling are otherwise on the right track.

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

src/components/Pagination/Pagination.tsx

mateacademy-ai-mentor · 2026-02-11T20:26:26Z

src/App.tsx

@@ -1,29 +1,51 @@
-import React from 'react';
+import React, { useState } from 'react';


The app must read and write the page and perPage query params via React Router so reloading restores the current view. Import and use appropriate router hooks (e.g. useSearchParams in react-router-dom v6 or useLocation/useHistory in v5) and useEffect to 1) read ?page and ?perPage on mount and initialize state, and 2) update the URL whenever currentPage or perPage change. This addresses the required behavior to save ?page=2&perPage=7 in the URL (checklist item #13).

mateacademy-ai-mentor · 2026-02-11T20:26:27Z

src/App.tsx

+  const [currentPage, setCurrentPage] = useState(1);
+  const [perPage, setPerPage] = useState(NumberPerPageItems.FIVE);


Initial perPage and currentPage state are hard-coded here. Instead initialize them from the URL query params (if present) on mount or compute initial state from location.search so the app applies ?page=...&perPage=... on page load. Use parseInt with sensible fallbacks (1 and 5).

src/App.tsx

mateacademy-ai-mentor · 2026-02-11T20:26:27Z

src/App.tsx

+        onPageChange={page => {
+          if (currentPage !== page) {
+            setCurrentPage(page);
+          }
+        }}


When the user changes page via onPageChange you update the local state correctly, but you also need to update the URL query params at that time so the current page is reflected in the address bar (and survives reload). Add code (e.g. in the same handler or in a useEffect watching currentPage/perPage) to push the new ?page=...&perPage=... to history. This satisfies the requirement that App listens to page changes and persists them to the URL (checklist item #13).

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override so you can keep progressing while you refine a few remaining details. You did a solid job implementing the core pagination behavior: the Pagination component is stateless, pages and prev/next links work correctly, the active page is highlighted with li.active, and the info text and per-page selector behave as required (including resetting to page 1 when perPage changes). The main missing requirement is URL persistence in src/App.tsx: you still need to read page and perPage from the query string on mount (using useSearchParams and useEffect) and then update those search params whenever currentPage or perPage change, so that reloading the page restores the current view. A good pattern is to initialize state like const [searchParams, setSearchParams] = useSearchParams(); const [currentPage, setCurrentPage] = useState(Number(searchParams.get('page') ?? '1')); and then in a useEffect keep ?page=...&perPage=... in sync with state. In src/components/Pagination/Pagination.tsx, you’re very close, but using React.FC without importing React (or FC) will cause a TypeScript error; you can fix this by adding import React from 'react'; or import type { FC } from 'react'; and using const Pagination: FC<Props> = (...) => { ... }. As a small robustness improvement, consider validating the URL params (e.g., clamp page to valid range and ensure perPage is one of the allowed values). Overall, your work shows good understanding of React state and component structure—tightening up the URL handling and the missing import will bring it fully in line with the requirements.

✨ 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-02-11T20:43:54Z

src/App.tsx

@@ -1,29 +1,59 @@
-import React from 'react';
+import React, { useState } from 'react';


Import React Router hooks and useEffect here. You need useSearchParams (from react-router-dom) and useEffect (from React) so the app can read/write page and perPage in the URL. This is required by the task's (*) URL persistence requirement.

mateacademy-ai-mentor · 2026-02-11T20:43:54Z

src/App.tsx

+}

 export const App: React.FC = () => {
+  const [currentPage, setCurrentPage] = useState(1);


Don't initialize currentPage as a fixed 1 — read it from the URL search params on mount (fallback to 1). Use parsed numeric value from searchParams.get('page') (e.g. Number(searchParams.get('page') ?? '1')).

mateacademy-ai-mentor · 2026-02-11T20:43:54Z

src/App.tsx


 export const App: React.FC = () => {
+  const [currentPage, setCurrentPage] = useState(1);
+  const [perPage, setPerPage] = useState(NumberPerPageItems.FIVE);


Same for perPage: initialize from the URL search params with fallback to NumberPerPageItems.FIVE. Parse to number (e.g. Number(searchParams.get('perPage') ?? String(NumberPerPageItems.FIVE))).

mateacademy-ai-mentor · 2026-02-11T20:43:54Z

src/App.tsx

+            onChange={e => {
+              setPerPage(Number(e.target.value));
+              setCurrentPage(1);


When the <select> changes you reset the page to 1 (correct). Also make sure the URL is updated to ?page=1&perPage=.... Recommended: centralize URL updates in a useEffect that writes search params whenever currentPage or perPage change, rather than updating the URL here and in other handlers.

mateacademy-ai-mentor · 2026-02-11T20:43:54Z

src/App.tsx

+        onPageChange={page => {
+          if (currentPage !== page) {
+            setCurrentPage(page);


The onPageChange handler updates currentPage (good). Ensure that this change is persisted to the URL as well (either by calling setSearchParams here or — recommended — letting a useEffect that watches currentPage/perPage do it). This is necessary so reloading restores the same page.

mateacademy-ai-mentor · 2026-02-11T20:43:54Z

src/components/Pagination/Pagination.tsx

+  onPageChange: (page: number) => void;
+};
+
+export const Pagination: React.FC<Props> = ({


You use the React.FC type on line 10 but React (or FC) is not imported in this file. Add an import at the top such as import React from 'react' or import type { FC } from 'react' and then use FC<Props> to avoid a TypeScript/compile error.

0-nira-0 added 2 commits February 11, 2026 20:07

solution

a67a226

fix page description of total elements

eb19977

mateacademy-ai-mentor suggested changes Feb 11, 2026

View reviewed changes

fix solution

7f0a753

0-nira-0 requested a review from mateacademy-ai-mentor February 11, 2026 20:22

mateacademy-ai-mentor suggested changes Feb 11, 2026

View reviewed changes

router is optional .-.

3955b5e

0-nira-0 requested a review from mateacademy-ai-mentor February 11, 2026 20:39

mateacademy-ai-mentor approved these changes Feb 11, 2026

View reviewed changes

		@@ -1,29 +1,51 @@
		import React from 'react';
		import React, { useState } from 'react';

		const [currentPage, setCurrentPage] = useState(1);
		const [perPage, setPerPage] = useState(NumberPerPageItems.FIVE);

		@@ -1,29 +1,59 @@
		import React from 'react';
		import React, { useState } from 'react';

Comments

Conversation

0-nira-0 commented Feb 11, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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