add tusk solution#1520
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement is still violated: onPageChange is being called even when the user clicks the already active page, which conflicts with checklist item #5 (the callback must fire only when the page actually changes). This is a functional behavior issue, not just style, and can cause unnecessary state updates and test failures.
Critical issues to fix:
- In
src/components/paganation/Pagination.tsx, inside the handler for numbered page links (around theonClick={() => onPageChange(page)}), add a guard so that the callback is only invoked whenpage !== currentPage. For example:onClick={() => { if (page !== currentPage) onPageChange(page); }}. - In
src/App.tsx, if you have a wrapper likehandlePageChange(page)that forwards directly tosetCurrentPage(page)oronPageChange(page), make sure it also respects this rule and doesn’t trigger redundant updates when the page is already selected.
Everything else in your implementation (stateless Pagination, prev/next disabling with li.disabled and aria-disabled, URL sync, per-page selector, item slicing, and info text formatting) looks solid and aligns well with the requirements. Once you add the equality check and ensure onPageChange never fires for the current page, your solution should fully meet the task standards. Keep up the good work—this is a small but important tweak to reach full compliance.
✨ 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
|
|
||
| <p className="lead" data-cy="info"> | ||
| Page 1 (items 1 - 5 of 42) | ||
| Page {currentPage} ( |
There was a problem hiding this comment.
This handler always calls onPageChange(page), even when page is equal to currentPage. Checklist item #5 requires that onPageChange be triggered only when the page actually changes. Consider short‑circuiting the handler when page === currentPage so that no callback is fired in that case.
| href={`#${page}`} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(page); |
There was a problem hiding this comment.
According to checklist item #5, onPageChange must be called only when the page actually changes. Here you always call onPageChange(page) even if page === currentPage, which can cause redundant updates. Add a guard (e.g. compare page with currentPage) before invoking the callback.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the Cypress end‑to‑end tests are currently all failing due to missing data-cy attributes, which is a critical blocker for the task completion. Functionally, your pagination and router integration look solid: Pagination is stateless, onPageChange is properly guarded, URL sync via useSearchParams works, and the per‑page selector and info text behave correctly. However, the tests expect specific attributes on elements, and without them the test suite cannot verify your otherwise correct behavior.
Critical issues to fix:
- Add
data-cy="pageLink"to each rendered page number link insrc/components/paganation/Pagination.tsx(the<a>elements inside your page<li>s). - Add
data-cy="prevLink"to the previous page control anddata-cy="nextLink"to the next page control. - Ensure the per-page
<select>inApp.tsxhasdata-cy="perPageSelector"exactly (it sounds like you already do this, but please double‑check spelling and casing).
Once these attributes are in place and the tests can find the elements, your implementation should pass the E2E suite and fully meet the requirements. The core logic looks very good, so this is mainly about wiring the DOM to the test harness correctly—nice work so far, and this should be a quick fix for you.
✨ 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.

DEMO LINK