Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one required starred feature is missing: the app does not yet sync pagination state with the URL query parameters using React Router. While your core pagination UI and behavior are solid (Pagination component props, highlighting, prev/next disabling, per-page selector, and info text all look correct), the task explicitly requires persisting page and perPage to the URL like ?page=2&perPage=7. To fix this, in src/App.tsx you should: 1) use React Router hooks (e.g. useSearchParams or useLocation + useNavigate) to read page and perPage from the query string on initial load and use those values to initialize currentPage and perPage (falling back to 1 and 5 if the params are missing or invalid), 2) update the query parameters whenever perPage changes (resetting page to 1 and pushing the new ?page=1&perPage=X), and 3) update the page query parameter whenever onPageChange updates the current page. As a small improvement, also prefer numeric values in your <option> elements (e.g. value={3} instead of "3") so types line up cleanly with your numeric state. Once you add this URL sync behavior, your solution will satisfy both the core and starred requirements, and the pagination state will be fully reflected in and restorable from the URL. Overall you’ve done a good job on the main pagination logic; aligning the Router integration will complete the task nicely.
✨ Please rate this review in the Platform's notification center1. 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 button2 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 tag3.
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.
Footnotes
src/App.tsx
Outdated
| import { useState } from 'react'; | ||
| import './App.css'; | ||
| import { getNumbers } from './utils'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const items = getNumbers(1, 42).map(n => `Item ${n}`); | ||
| import { Pagination } from './components/Pagination'; |
There was a problem hiding this comment.
The app currently has no Router/query-param handling. To satisfy the starred requirement you need to import and use React Router hooks (e.g. useSearchParams or useLocation/useNavigate) so you can read ?page and ?perPage on load and keep them in sync with state.
src/App.tsx
Outdated
| const [pegeAmount, setAmount] = useState(5); | ||
| const [currentPage, setCurrent] = useState(1); |
There was a problem hiding this comment.
currentPage and perPage are initialized with fixed defaults here. To support loading from the URL you should parse query params and initialize these states from them (fallback to 1 and 5 if params are missing/invalid).
| onChange={e => { | ||
| setAmount(Number(e.target.value)); | ||
| setCurrent(1); | ||
| }} |
There was a problem hiding this comment.
When perPage changes you reset the page to 1 (good), but you also need to update the URL query params here so the new perPage and page=1 are reflected in the address bar (use the Router navigation hook).
| onPageChange={newPege => { | ||
| setCurrent(newPege); | ||
| }} |
There was a problem hiding this comment.
When onPageChange sets the new page you update state correctly — also update the URL query params here so ?page stays in sync with the selected page (use the Router navigation hook).
src/App.tsx
Outdated
| setCurrent(1); | ||
| }} | ||
| > | ||
| <option value="3">3</option> |
There was a problem hiding this comment.
The values are strings (e.g. "3"). It's safer to use numeric values (e.g. value={3}) to match the numeric state type and avoid any unexpected coercion when comparing value/option.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because all core requirements of the pagination task are implemented and working: the stateless Pagination component behaves correctly, App wires it up with React Router, and page/perPage are synced to and restored from the URL. The prev/next links are correctly disabled on boundaries, the current page is highlighted, the info text in data-cy="info" is correctly formatted, and the per-page selector with options 3, 5, 10, and 20 behaves as specified.
The remaining issues are real but not blocking: in src/App.tsx, URL-derived page and perPage values are not validated or clamped, so ?page=999 or ?perPage=0 could lead to odd or misleading ranges, and a perPage value not matching any <option> (e.g. 7) makes the select controlled without a matching option. In src/components/Pagination/Pagination.tsx, it would be safer to guard against perPage <= 0 before computing pages = Math.ceil(total / perPage), and optionally omit aria-disabled when it would be false. None of these break the required features, but they are good robustness improvements to consider: normalize perPage to allowed values, clamp currentPage to [1, pages], and ensure pages is at least 1 when rendering.
Overall, you met the functional checklist very well, and with a bit of extra validation your solution would be production-ready-level solid.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK