develop by fedechkova · Pull Request #1478 · mate-academy/react_pagination

fedechkova · 2026-04-17T20:42:26Z

mateacademy-ai-mentor

Review Summary

Changes are needed before this can be considered complete, mainly due to routing and import issues that will break the app and tests at runtime.

In src/App.tsx and src/index.tsx, BrowserRouter and useSearchParams are imported from react-router, but React Router v6 exposes these from react-router-dom; with the current imports the router and hooks will be undefined, so URL ?page / ?perPage syncing will not work at all. Please update those imports to use react-router-dom so the checklist requirement to read and store pagination state in the URL is actually satisfied.
In src/App.tsx, you import Pagination from ./components/Pagination, but the actual file is ./components/Pagination/Pagination.tsx; unless you add a components/Pagination/index.ts that re‑exports Pagination, this will cause a module‑not‑found error, so either add that barrel file or point the import directly at ./components/Pagination/Pagination.
In src/components/Pagination/Pagination.tsx, useSearchParams should also come from react-router-dom rather than react-router, otherwise the hook will fail at runtime and any URL‑dependent behavior inside the component will break.
The functional pagination behavior (stateless component, correct page calculation, disabled prev/next, info text, per‑page selector, URL persistence logic) is otherwise well implemented, but to keep tests robust you should also double‑check that the HTML structure exactly matches what the Cypress tests expect (especially whether page links should be <a> instead of <button> with the same data-cy attributes).
Once you fix the imports and ensure the Pagination module resolves correctly, re‑run the test suite to confirm that URL syncing and pagination rendering behave correctly across all cases.

Overall, you are very close: the core logic and UI are in good shape, and resolving these routing/import details will make the solution solid and test‑friendly.

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

mateacademy-ai-mentor · 2026-04-17T20:45:50Z

 import './App.css';
 import { getNumbers } from './utils';
+import { Pagination } from './components/Pagination';
+import { useSearchParams } from 'react-router';


The useSearchParams hook is imported from react-router here, but it should be imported from react-router-dom. Change to: import { useSearchParams } from 'react-router-dom'; — otherwise the routing hook will be undefined and URL params won't be read/saved as required by the task.

mateacademy-ai-mentor · 2026-04-17T20:45:50Z

@@ -1,25 +1,56 @@
 import React from 'react';
 import './App.css';
 import { getNumbers } from './utils';


The BrowserRouter is imported from react-router here; it must come from react-router-dom. Change to: import { BrowserRouter } from 'react-router-dom'; so the router works and useSearchParams/URL handling functions correctly.

mateacademy-ai-mentor · 2026-04-17T20:45:50Z

 import React from 'react';
 import './App.css';
 import { getNumbers } from './utils';
+import { Pagination } from './components/Pagination';


Importing Pagination with ./components/Pagination will only work if there's an index barrel file re-exporting the component. The actual file is ./components/Pagination/Pagination.tsx; either import that explicit path or add an index.ts to re-export Pagination to avoid a module-not-found error.

mateacademy-ai-mentor · 2026-04-17T20:45:50Z

+
+type PaginationProps = {
+  currentPage?: number;
+  total: number;


Importing useSearchParams from 'react-router' is incorrect. The hook is provided by 'react-router-dom' in typical React Router v6 usage. Change to import { useSearchParams } from 'react-router-dom'; so the hook is resolved at runtime.

mateacademy-ai-mentor · 2026-04-17T20:45:50Z

+import React from 'react';
+
+type PaginationProps = {
+  currentPage?: number;


Ensure the import path for the Pagination component is resolvable. You're importing from './components/Pagination' — that only works if there is an index barrel file exporting Pagination. If you don't have such a re-export, import directly from ./components/Pagination/Pagination or add an index file that re-exports the component.

mateacademy-ai-mentor · 2026-04-17T20:45:50Z

+  const pages = Array.from({ length: totalPages }, (_, i) => i + 1);
+
+  const isFirstPage = currentPage === 1;


You restrict itemsPerPage to allowedPerPageValues. That means arbitrary perPage values in the URL (for example ?perPage=7) will be ignored and fall back to 5. If the task expects applying arbitrary perPage values from the URL, remove this filter; if you want to keep the selector limited to specific options, document/accept that URL values outside the allowed set will revert to default.

mateacademy-ai-mentor · 2026-04-17T20:45:51Z

-export const Pagination = () => {};
+import React from 'react';
+
+type PaginationProps = {


In index.tsx you import BrowserRouter from 'react-router'. Like the hook above, BrowserRouter should be imported from 'react-router-dom'. Update the import to import { BrowserRouter } from 'react-router-dom'; to ensure routing works.

mateacademy-ai-mentor · 2026-04-17T20:45:51Z

+          <button
+            type="button"
+            data-cy="pageLink"
+            className="page-link"
+            onClick={() => handlePageChange(page)}


The page number links are rendered as <button> elements. The task states to keep the HTML structure; if the original markup expected anchors for page links, replacing them with <button> changes the structure and may break tests that assert the element type. Consider using <a> with appropriate href (and preventing default on click) to preserve the original structure if required.

mateacademy-ai-mentor · 2026-04-17T20:45:51Z

+import ReactDOM from 'react-dom/client';
+import { BrowserRouter } from 'react-router';
+import App from './App';



Importing useSearchParams from 'react-router' is incorrect. The hook comes from 'react-router-dom'. This prevents the app from using React Router search params and so the ?page and ?perPage URL behavior will not work.

mateacademy-ai-mentor · 2026-04-17T20:45:51Z

-import { createRoot } from 'react-dom/client';
+import React from 'react';
+import ReactDOM from 'react-dom/client';
+import { BrowserRouter } from 'react-router';


Importing BrowserRouter from 'react-router' is wrong; it should be imported from 'react-router-dom'. Because of this the router wrapper won't be set up correctly and URL search params won't be managed as required.

mateacademy-ai-mentor

Review Summary

I am requesting changes because the end‑to‑end Cypress tests are currently all failing: they cannot find any of the expected pagination elements in the rendered DOM, which means the task’s functional requirements are not fully met. The logs show that selectors like [data-cy="item"], [data-cy="pageLink"], [data-cy="perPageSelector"], [data-cy="prevLink"], and [data-cy="nextLink"] are missing, so even though your components are implemented, the pagination UI is not actually visible to the test runner. In src/App.tsx, double‑check the part where you render the list of visible items (around the visibleItems.map(...) call) and ensure each rendered item element has the exact attribute data-cy="item" and is rendered on the initial route that Cypress opens. Similarly, in src/components/Pagination/Pagination.tsx, confirm that the previous/next links and each page link are rendered unconditionally within the main page, with data-cy="prevLink", data-cy="nextLink", and data-cy="pageLink" on the clickable <a> elements, and that the per‑page <select> with data-cy="perPageSelector" is present on initial load. Because Cypress cannot see these elements at all, verify that App is what you mount at the root route in src/index.tsx (no extra routing layer that would hide the pagination page), then run npm test locally until all page.spec.js tests pass. [CHECKLIST ITEM #20] As a smaller improvement once tests are green, you may want to guard the pagination logic for the edge case where total === 0 so that you don’t render or navigate to a non‑existent page, but this is not what’s currently blocking the task. You’ve done strong work wiring the stateless Pagination component and the URL search‑param syncing; once you align what’s actually rendered on the initial route with what Cypress expects, your solution should be in excellent shape.

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

feat: implement pagination with perPage selector and URL params

66cad36

mateacademy-ai-mentor suggested changes Apr 17, 2026

View reviewed changes

fix: use anchor for page links and update imports

8dc14f9

fedechkova requested a review from mateacademy-ai-mentor April 18, 2026 11:18

mateacademy-ai-mentor suggested changes Apr 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

develop#1478

develop#1478
fedechkova wants to merge 2 commits into
mate-academy:masterfrom
fedechkova:develop

fedechkova commented Apr 17, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor Apr 17, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		const pages = Array.from({ length: totalPages }, (_, i) => i + 1);

		const isFirstPage = currentPage === 1;

Conversation

fedechkova commented Apr 17, 2026

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!

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!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants