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 there are still critical issues that break required pagination behavior and violate explicit checklist items. In src/App.tsx, the items are sliced with items.slice(start, start + end) and the end capping (if (end > items.length) end = items.length) happens after the slice; this means the page shows the wrong items and can disagree with the info text, breaking the core requirement to show only the current page’s items [CHECKLIST ITEM #1, #11]. To fix this, first compute a correct capped end index (for example const end = Math.min(start + perPage, items.length)), then call items.slice(start, end) and use that same end in the info text.
In src/components/Pagination/Pagination.tsx, the component keeps pages in state via useState/useEffect even though they are derived from total and perPage, which violates the requirement that Pagination be stateless and also risks stale pages when total changes [CHECKLIST ITEM #1, #4, #20]; compute the pages directly from props on each render instead and remove the effect/state. The currentPage prop is required instead of optional with a default of 1, so you should change the typing to currentPage?: number and set a default value in the function parameter [CHECKLIST ITEM #2, #20]. Finally, all click handlers (prev, next, and page links) call onPageChange unconditionally: they should prevent default navigation and only call the callback when the new page is within bounds and different from the current page (e.g., guard prev with currentPage > 1, next with currentPage < pagesCount, and skip calling when page === currentPage) [CHECKLIST ITEM #6, #8, #9]. Once you address these functional issues, the overall structure of your UI and wiring in App will fit the requirements well, so you’re very close—these changes will make the behavior robust and aligned with the spec.
✨ 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
| const [total] = useState(items.length); | ||
| const start = (currentPage - 1) * perPage; | ||
| let end = start + perPage; | ||
| const filteredItems = items.slice(start, start + end); |
There was a problem hiding this comment.
This line computes filteredItems using items.slice(start, start + end). slice expects (start, endIndex) — using start + end is incorrect and will return the wrong items. Use items.slice(start, end) (after ensuring end is the correct capped end index). This currently breaks the requirement to show only items for the current page (checklist #1).
src/App.tsx
Outdated
| let end = start + perPage; | ||
| const filteredItems = items.slice(start, start + end); | ||
|
|
||
| if (end > items.length) { |
There was a problem hiding this comment.
The end capping (if (end > items.length) end = items.length) happens after filteredItems is computed. Because of that the slice uses an incorrect end and the rendered items will not match the info text. Move the slice to after you compute/cap end (or compute const sliceEnd = Math.min(start + perPage, items.length) and slice with that).
src/App.tsx
Outdated
| const filteredItems = items.slice(start, start + end); | ||
|
|
||
| if (end > items.length) { | ||
| end = items.length; |
There was a problem hiding this comment.
This line sets the capped end. Good to cap here, but see previous comment: the cap must be applied before slicing. Consider replacing let end = start + perPage; + this cap with const end = Math.min(start + perPage, items.length); so the value is correct and can be used immediately for slicing and info.
|
|
||
| <p className="lead" data-cy="info"> | ||
| Page 1 (items 1 - 5 of 42) | ||
| Page {currentPage} (items {start + 1} - {end} of {items.length}) |
There was a problem hiding this comment.
The info text (Page {currentPage} (items {start + 1} - {end} of {items.length})) is correct in format (checklist #11), but due to the slice bug the visible items may not match this range. After fixing the slice (use the capped end), this info will correctly reflect the rendered items.
| type Props = { | ||
| total: number; | ||
| perPage: number; | ||
| currentPage: number; |
There was a problem hiding this comment.
The currentPage prop is defined as required here. The checklist requires currentPage to be optional with a default value of 1 (checklist item #20). Make it currentPage?: number in the type and provide a default in the function signature (e.g. currentPage = 1).
| currentPage, | ||
| onPageChange, | ||
| }: Props) => { | ||
| const [pages, setPages] = useState([1]); |
There was a problem hiding this comment.
You store pages in component state. The task asks for a stateless Pagination that shows items for the current page (checklist item #1). pages is derived from total and perPage and should be computed on render (e.g. use Array.from(...)) instead of using useState/useEffect here. That also lets you remove useState/useEffect imports if unused.
| } | ||
|
|
||
| setPages(pagesArr); | ||
| }, [perPage]); |
There was a problem hiding this comment.
The effect that computes pages depends only on perPage but uses total as well. If you keep this effect, add total to dependencies; otherwise prefer computing pages directly from total and perPage without effect/state. As written this can leave pages stale when total changes (checklist item #4).
| className="page-link" | ||
| href="#prev" | ||
| aria-disabled={currentPage === 1 ? true : false} | ||
| onClick={() => onPageChange(currentPage - 1)} |
There was a problem hiding this comment.
The prev link onClick calls onPageChange(currentPage - 1) unconditionally. That can call the callback with an out-of-range page (e.g. 0) and violates the requirement that onPageChange only fires when the page actually changes (checklist items #6 and #8). Guard this call (e.g. if currentPage > 1) and prevent default browser navigation.
| data-cy="pageLink" | ||
| className="page-link" | ||
| href="#1" | ||
| onClick={() => onPageChange(page)} |
There was a problem hiding this comment.
Each page link calls onPageChange(page) unconditionally. According to checklist item #6, onPageChange must be triggered only if the page was changed — avoid calling it when page === currentPage. Add a guard so clicking the current page does not invoke the callback.
| className="page-link" | ||
| href="#next" | ||
| aria-disabled={currentPage === pages.length ? true : false} | ||
| onClick={() => onPageChange(currentPage + 1)} |
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 mandatory pagination requirements are implemented correctly and the core behavior works as expected. The App component now computes a proper start and capped end index, uses them consistently for both slicing and the info text, wires onPageChange to update currentPage, and resets to page 1 when perPage changes. The Pagination component is stateless, derives pagesCount from total and perPage, handles prev/next with proper bounds checks and preventDefault(), and correctly applies li.active / li.disabled and the required data-cy attributes.
A few minor improvements you could consider (not required for this task) are: using the total prop instead of items.length when rendering the info text, handling the total = 0 edge case more explicitly, and normalizing aria-disabled values to strings. The optional URL synchronization with React Router for ?page= and ?perPage= is also not implemented, but it was explicitly optional and doesn’t block this submission. Overall, this is a solid, clean implementation of the required pagination features—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
3 participants
No description provided.