Solution#1479
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this solution can be accepted, mainly around the starred React Router requirement and the onPageChange behavior.
-
Critical: URL query synchronization is missing. The task explicitly requires using React Router to read
?pageand?perPagefrom the URL on load and to keep them in sync when they change. Right now,currentPageandperPageare initialized with fixed values inApp.tsx(and related components likeItemList,PerPageSelector,PageInfo) and there is no use ofreact-router-domhooks such asuseLocation/useNavigateoruseSearchParams. You should:- Parse
pageandperPagefromlocation.search(oruseSearchParams) on initial render, validate them (clamppageto[1, totalPages]andperPageto one of3, 5, 10, 20), and use those values to initialize state. - In your page change handler (e.g.
handlePageChangeinApp.tsx) and per-page change handler (handlePerPageChange), update the URL query string via React Router (e.g.navigate({ search: ... })) so that?page=...&perPage=...always reflects the current state and persists across reloads.
- Parse
-
Critical:
onPageChangeis fired even when clicking the current page. InPagination.tsx(and any wrapper that uses it, such asApp.tsx), the click handler for page links always callsonPageChange(page)even whenpage === currentPage, which violates the requirement that the callback should be triggered only if the page actually changes. Add a simple guard likeif (page !== currentPage) onPageChange(page);inside the click handler, and likewise ensure that any higher-level handler inApp.tsxalso checks before updating state. -
Minor improvements (non-blocking): Consider renaming the
VisiblevisibleItemsinterface inItemList.tsxto something clearer likeItemListProps, and when you add query-string support, make sure your parsing is robust and clamps invalid values to safe defaults.
Once you implement URL syncing with React Router and adjust onPageChange so it only fires on actual page changes, the rest of your pagination logic and UI look solid and should meet the task requirements.
✨ 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
| @@ -0,0 +1,15 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
The task requires using React Router to save and apply query params (e.g. ?page=2&perPage=7). There are no react-router imports or usage in this file — you should parse page and perPage from the URL on load and keep the URL in sync when they change.
| {item} | ||
| </li> | ||
| ))} | ||
| </ul> |
There was a problem hiding this comment.
currentPage is initialized to 1 but the spec requires applying ?page from the URL on load. Consider reading page from the query (via useLocation / URLSearchParams) and using it to initialize or set currentPage (validate/normalize the value).
| </li> | ||
| ))} | ||
| </ul> | ||
| ); |
There was a problem hiding this comment.
perPage is initialized to 5 but must also be loaded from the URL if ?perPage is present. Parse perPage from the query string on load and ensure the selector reflects it.
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(page); |
There was a problem hiding this comment.
The page link onClick handler always calls onPageChange(page) even when page === currentPage. The requirement says: "onPageChange callback should be triggered only if page was changed". Add a guard so you only call onPageChange when page !== currentPage.
| onPageChange, | ||
| }) => { |
There was a problem hiding this comment.
The component/app does not read ?page and ?perPage from the URL on load. The task requires using React Router to apply URL params on page load (e.g. ?page=2&perPage=7). Instead of initializing state with fixed values, read search params and initialize currentPage and perPage from them (with sensible fallbacks).
| return ( | ||
| <div className="form-group row"> | ||
| <div className="col-3 col-sm-2 col-xl-1"> | ||
| <select |
There was a problem hiding this comment.
currentPage is initialized with a fixed value (useState(1)) but the spec requires applying ?page from the URL on page load. Parse search params and initialize or update currentPage from the URL (validate the value and ensure it fits within available pages).
| <div className="form-group row"> | ||
| <div className="col-3 col-sm-2 col-xl-1"> | ||
| <select | ||
| data-cy="perPageSelector" |
There was a problem hiding this comment.
perPage is initialized with a fixed value (useState(5)) but must be read from ?perPage when present. On load, parse and apply perPage from the query string so the selector and pagination reflect the URL.
| > | ||
| <option value="3">3</option> | ||
| <option value="5">5</option> |
There was a problem hiding this comment.
handlePageChange only updates component state. Per the requirements, page changes must be saved in the URL (e.g. ?page=2&perPage=5). Update this handler to push/replace query params via React Router so the URL reflects the current page.
| <option value="20">20</option> | ||
| </select> | ||
| </div> |
There was a problem hiding this comment.
handlePerPageChange resets currentPage to 1 which is correct, but it doesn't update the URL. When perPage changes you must update the query string (e.g. ?page=1&perPage=10) so the selection persists and will be applied on reload.
| @@ -0,0 +1,32 @@ | |||
| import React from 'react'; | |||
| interface PerPageSelectorProps { | |||
There was a problem hiding this comment.
Minor: the interface name VisiblevisibleItems in ItemList appears to be a typo and reduces readability. Consider renaming to e.g. ItemListProps (no functional impact but improves clarity).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a core requirement of the task—synchronizing pagination with the URL via React Router—is not yet implemented, which means ?page=...&perPage=... is neither read on load nor persisted when state changes. The main critical issue is that in src/App.tsx, src/components/ItemList/ItemList.tsx, src/components/PageInfo.tsx, and src/components/Pagination/Pagination.tsx you still initialize currentPage and perPage with fixed defaults instead of parsing them from the query string and validating them (clamp page to [1, totalPages] and ensure perPage is one of 3, 5, 10, 20) as required [CHECKLIST ITEM #13, #14, #25]. Closely related, none of these components currently use React Router hooks (useSearchParams or useLocation + useNavigate) to read page/perPage from location.search on first render or to update the URL when pagination changes, so state does not persist across reloads or deep links [CHECKLIST ITEM #15, #17, #26]. In App.tsx (and wherever you handle page/perPage at the top level), your handlePageChange should guard against no-op updates with a check like if (page === currentPage) return; and then both update state and sync the query string, and likewise handlePerPageChange should validate the new value, reset the page to 1, avoid redundant updates, and write ?page=1&perPage=... back to the URL [CHECKLIST ITEM #6, #11, #12, #27, #28, #33]. These missing behaviors are considered blocking, because the assignment explicitly requires URL-controlled initial state and URL persistence for pagination, not just internal state management. As a minor, non-blocking improvement, consider renaming the props interface VisiblevisibleItems (e.g., in ItemList/PageInfo) to something clearer like ItemListProps to improve readability. Once you wire pagination state through React Router, validate/clamp query parameters, and add the guards to avoid redundant state/URL updates, your otherwise solid pagination UI will fully meet the task requirements, and your work will be in great shape.
✨ 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
| @@ -0,0 +1,20 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
| total, | ||
| }) => { |
There was a problem hiding this comment.
currentPage and perPage are initialized with fixed defaults here. Per requirements you must initialize these values from the URL query string on first render (parse page and perPage, validate them: clamp page to [1, totalPages] and ensure perPage is one of [3,5,10,20]) so the initial state reflects ?page=...&perPage=... (checklist items #13, #14, #25).
| @@ -0,0 +1,20 @@ | |||
| import React from 'react'; | |||
|
|
|||
There was a problem hiding this comment.
Minor: the props interface name VisiblevisibleItems is confusing. Consider renaming it to ItemListProps (or similar) for clarity — this doesn't affect functionality but improves readability.
| @@ -1,110 +1,53 @@ | |||
| import React from 'react'; | |||
| import React, { useState } from 'react'; | |||
There was a problem hiding this comment.
| const [currentPage, setCurrentPage] = useState(1); | ||
| const [perPage, setPerPage] = useState(5); |
There was a problem hiding this comment.
Don't initialize currentPage and perPage with hard-coded values. Parse page and perPage from the URL on initial render, validate them (clamp page to [1, totalPages] and ensure perPage is one of 3,5,10,20), and use those validated values to initialize state (lazy initializer or effect). This implements checklist items #13, #14 and #25.
| perPage, | ||
| currentPage = 1, | ||
| onPageChange, | ||
| }) => { |
There was a problem hiding this comment.
Don't hardcode perPage to 5. Parse perPage from the URL on initial render (allowed values: 3, 5, 10, 20), validate it (fall back to 5 if invalid), and use that parsed value to initialize this state. The selector already provides these options but initial state must reflect the URL (checklist items #11, #14, #25).
| pages.push(i); | ||
| } | ||
|
|
There was a problem hiding this comment.
The App-level handler should avoid unnecessary updates: add a guard if (page === currentPage) return; before calling setCurrentPage(...). Also, when the page actually changes, update the URL query string (e.g. via navigate({ search: ... }) or setSearchParams) so ?page=...&perPage=... stays in sync (checklist items #6, #26, #27, #28, #33).
| <ul className="pagination"> | ||
| <li className={cn('page-item', { disabled: currentPage === 1 })}> | ||
| <a |
There was a problem hiding this comment.
When perPage changes, only update state if the new value differs from the current one, reset page to 1, and update the URL query string accordingly. Also validate the new perPage is one of [3,5,10,20] before applying it. This keeps state and URL synchronized and satisfies checklist items about per-page selector and URL persistence (#11, #12, #24, #26).
| @@ -0,0 +1,15 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
The task requires reading and writing pagination state from/to the URL using React Router (useSearchParams or useLocation/useNavigate). This file currently has no react-router usage: import and use the appropriate hooks so you can parse ?page and ?perPage on load and update the query string when the user changes page or perPage (check checklist items #13, #14, #15, #16, #17, #26).
| </ul> | ||
| ); |
There was a problem hiding this comment.
currentPage and perPage are initialized here with static defaults. Per checklist items #13/#25 you must instead initialize them from the URL query string on initial render, validating values (clamp page to [1, totalPages] and ensure perPage is one of 3,5,10,20). Use parsed & validated values to set initial state instead of always starting with 1 and 5.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing, but there are a few important areas to improve for full requirements compliance. You did a solid job on the visual pagination UI: the page list, active/disabled states, data-cy attributes, and the PerPageSelector markup and behavior all match the task expectations and are nicely separated into stateless components. The main missing piece is URL synchronization: in App.tsx (and related top-level state), currentPage and perPage are initialized with fixed defaults instead of being parsed from ?page and ?perPage using React Router hooks like useSearchParams or useLocation + useNavigate, and changes are not written back to the URL, which breaks deep links and reload persistence [CHECKLIST ITEM #1]. When you add that, remember to validate and clamp values (e.g., clamp page to [1, totalPages] and ensure perPage is one of 3, 5, 10, 20), and to guard handlers like handlePageChange/handlePerPageChange against no-op updates before updating state and search params [CHECKLIST ITEM #2]. Another small improvement is to rename confusing prop interfaces such as VisiblevisibleItems to something clearer like ItemListProps to improve readability and maintainability. Overall, your component structure and UI behavior are on the right track; once you wire the pagination state to the URL with proper validation, your solution will fully meet the assignment’s requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @@ -1,110 +1,53 @@ | |||
| import React from 'react'; | |||
| import React, { useState } from 'react'; | |||
There was a problem hiding this comment.
The component currently only imports React. To meet the requirement that ?page=...&perPage=... is read on load and persisted, import and use React Router hooks (e.g., useSearchParams or useLocation + useNavigate) so you can read query params on initial render and update them when pagination changes.
| const items = getNumbers(1, 42).map(n => `Item ${n}`); | ||
|
|
||
| export const App: React.FC = () => { | ||
| const [currentPage, setCurrentPage] = useState(1); |
There was a problem hiding this comment.
Do not initialize currentPage with a fixed default here. Instead parse page from the URL on initial render, validate and clamp it to the range [1, totalPages], and use that validated value as the initial state.
|
|
||
| export const App: React.FC = () => { | ||
| const [currentPage, setCurrentPage] = useState(1); | ||
| const [perPage, setPerPage] = useState(5); |
There was a problem hiding this comment.
Do not initialize perPage with a fixed default here. Parse perPage from the URL, ensure it is one of the allowed values (3, 5, 10, 20), and default to 5 only if the query value is missing/invalid.
| const startIndex = (currentPage - 1) * perPage; | ||
| const lastIndex = Math.min(startIndex + perPage, items.length); |
There was a problem hiding this comment.
Computing startIndex/lastIndex before validating / clamping currentPage can produce incorrect slices. Make sure currentPage is validated against totalPages first, then compute startIndex and lastIndex from the validated values.
| const handlePageChange = (page: number) => { | ||
| setCurrentPage(page); | ||
| }; |
There was a problem hiding this comment.
handlePageChange should guard against no-op updates (e.g., if (page === currentPage) return;) and also synchronize the new page to the URL (use setSearchParams or navigate to write ?page=...&perPage=...). Right now it only calls setCurrentPage, so URL persistence is missing.
| const handlePerPageChange = (pages: number) => { | ||
| setPerPage(pages); | ||
| setCurrentPage(1); | ||
| }; |
There was a problem hiding this comment.
handlePerPageChange needs to validate the incoming value (allow only 3,5,10,20), avoid redundant updates, reset page to 1, and persist ?page=1&perPage=... to the URL. Currently it updates state but does not validate or update the query string.
| <Pagination | ||
| total={items.length} | ||
| perPage={perPage} | ||
| currentPage={currentPage} | ||
| onPageChange={handlePageChange} |
There was a problem hiding this comment.
You pass currentPage and perPage into Pagination, but because App doesn't read initial query params or write updates to the URL, deep links and reloads will not reflect the correct pagination state. Integrate React Router syncing so the values rendered come from the validated query params and updates modify the URL.
| @@ -1,110 +1,53 @@ | |||
| import React from 'react'; | |||
| import React, { useState } from 'react'; | |||
| import './App.css'; | |||
There was a problem hiding this comment.
Rename the props interface VisiblevisibleItems to a clearer name like ItemListProps for readability (this is a non-blocking suggestion but improves clarity).
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const items = getNumbers(1, 42).map(n => `Item ${n}`); | ||
|
|
||
| export const App: React.FC = () => { |
There was a problem hiding this comment.
Defaulting currentPage to 1 inside Pagination is acceptable for a stateless component, but it only meets the requirement when the App passes a validated value coming from the URL. Ensure the App provides a validated currentPage from parsed query params so the default isn't relied on unexpectedly.
| @@ -1 +1,78 @@ | |||
| export const Pagination = () => {}; | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
You need to read initial pagination state from the URL and persist it back when it changes. Import and use React Router hooks (e.g., useSearchParams or useLocation + useNavigate) here to parse ?page and ?perPage on first render and to update the query string when state changes. This is required by the task (synchronize pagination with the URL).
| onPageChange, | ||
| }) => { |
There was a problem hiding this comment.
Avoid initializing currentPage and perPage with fixed defaults here. Instead parse page and perPage from the query string, validate them (clamp page between 1 and totalPages and ensure perPage is one of [3,5,10,20]), and use those validated values as initial state. The task requires applying query params on page load rather than always starting from fixed defaults.
| const totalPages = Math.ceil(total / perPage); | ||
| const pages = []; | ||
|
|
There was a problem hiding this comment.
startIndex/lastIndex/visibleItems are computed from the current state — make sure they are computed after you parse and validate page/perPage from the URL. If you don't clamp page to [1, totalPages], these indices can be out of expected range and break the displayed info/items.
| pages.push(i); | ||
| } |
There was a problem hiding this comment.
Guard against no-op page changes and synchronize the URL. handlePageChange should do if (page === currentPage) return;, then update state and write the new ?page=...&perPage=... to the URL (e.g., via useNavigate/setSearchParams). The task requires triggering onPageChange only when the page actually changes and persisting it to the URL.
| <ul className="pagination" data-cy="pagination"> | ||
| <li className={cn('page-item', { disabled: currentPage === 1 })}> | ||
| <a |
There was a problem hiding this comment.
Validate the incoming perPage, avoid redundant updates, reset the page to 1 and persist ?page=1&perPage=... to the URL. handlePerPageChange should check that pages is one of [3,5,10,20], and skip updating if the selected value equals current perPage. Per the requirements, after changing perPage you must always show the 1st page and update the query string.
| export const Pagination: React.FC<PaginationProps> = ({ | ||
| total, | ||
| perPage, | ||
| currentPage = 1, |
There was a problem hiding this comment.
Pagination is currently implemented as a stateless component which is good, but relying on its internal default currentPage = 1 (here) is not sufficient: the top-level App must supply validated currentPage/perPage parsed from the URL. Ensure URL parsing/validation happens at the top level so Pagination receives correct values.
| @@ -1 +1,78 @@ | |||
| export const Pagination = () => {}; | |||
| import React from 'react'; | |||
| import cn from 'classnames'; | |||
There was a problem hiding this comment.
Minor readability suggestion: rename the VisiblevisibleItems interface to something clearer like ItemListProps (or similar). This makes the component API clearer and was recommended in the checklist as a low-priority improvement.
| @@ -0,0 +1,20 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
The App component currently doesn't import or use any React Router hooks. The task requires reading ?page and ?perPage from the URL on load and persisting changes back to the URL. Import and use useSearchParams or useLocation + useNavigate in App to read/write the query string so pagination state survives reloads/deep links.
| currentPage, | ||
| startIndex, | ||
| lastIndex, | ||
| total, |
There was a problem hiding this comment.
Do not hardcode the initial currentPage with useState(1). You must parse page from the query string on initial render, clamp it to the valid range [1, totalPages], and initialize state from that parsed value (fallback to 1 only if the query param is missing/invalid).
| startIndex, | ||
| lastIndex, | ||
| total, | ||
| }) => { |
There was a problem hiding this comment.
Do not hardcode the initial perPage with useState(5). Parse perPage from the query string and ensure it's one of the allowed options [3, 5, 10, 20]. Use a validated value (or fallback to a default) when initializing state so the app respects ?perPage=... on load.
| export const PageInfo: React.FC<PageInfoProps> = ({ | ||
| currentPage, | ||
| startIndex, | ||
| lastIndex, |
There was a problem hiding this comment.
Pagination is implemented as a stateless component, which is good. However, ensure the top-level App is the single source of truth for currentPage/perPage initialized from the URL; otherwise the UI may not reflect query params on load even though Pagination has a default currentPage=1.
| @@ -0,0 +1,20 @@ | |||
| import React from 'react'; | |||
|
|
|||
There was a problem hiding this comment.
Minor: the props interface name VisiblevisibleItems is confusing. Consider renaming it to ItemListProps (or similar) for readability. This is non-blocking but improves maintainability.
| {item} | ||
| </li> | ||
| ))} | ||
| </ul> |
There was a problem hiding this comment.
You initialize currentPage with a fixed default. Per the task you must read page from the URL on initial load and validate/clamp it (to the range [1, totalPages]). Use React Router (e.g., useSearchParams or useLocation+useNavigate) to read the query params instead of hard-coded defaults.
| </li> | ||
| ))} | ||
| </ul> | ||
| ); |
There was a problem hiding this comment.
You initialize perPage with a fixed default. The app must read perPage from the URL on load and validate it (ensure it's one of 3, 5, 10, 20). Replace the fixed default with a value parsed from the query string.
| <li data-cy="item" key={item}> | ||
| {item} | ||
| </li> | ||
| ))} |
There was a problem hiding this comment.
The top-level component should read page and perPage from the URL on first render (using React Router hooks), validate them (clamp page to [1, totalPages], ensure perPage ∈ {3,5,10,20}), and apply them to state so deep links work. Add that logic here before computing visible items.
| @@ -0,0 +1,15 @@ | |||
| import React from 'react'; | |||
| interface VisiblevisibleItems { | |||
There was a problem hiding this comment.
Interface name VisiblevisibleItems is confusing. Consider renaming to something clearer like ItemListProps to improve readability (this is a minor, non-blocking suggestion from the checklist).
| value={perPage} | ||
| onChange={e => onPerPageChange(+e.target.value)} |
There was a problem hiding this comment.
The select and options are implemented correctly. Ensure the parent (App) validates that the perPage prop is one of the allowed values (3, 5, 10, 20) and resets to a valid default if not. If perPage is out of that set the select may have no matching option selected — validation/clamping should happen upstream per task requirements.
DEMO LINK