Skip to content

Solution#1480

Open
aholubko wants to merge 1 commit into
mate-academy:masterfrom
aholubko:develop
Open

Solution#1480
aholubko wants to merge 1 commit into
mate-academy:masterfrom
aholubko:develop

Conversation

@aholubko
Copy link
Copy Markdown

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because the mandatory React Router integration for syncing pagination with URL query parameters is missing, which blocks a core requirement of the task. In src/App.tsx, currentPage and perPage are initialized with hardcoded useState defaults instead of reading ?page and ?perPage from the URL on initial load, and there is no logic to update the URL when these values change. The same applies to src/components/Pagination/Pagination.tsx, where the component and its consumers do not use react-router-dom hooks (such as useSearchParams, or useLocation with useNavigate) to read or write page and perPage in the query string. To fix this, wrap the app in a BrowserRouter at the root, then in App.tsx derive initial currentPage/perPage from search params (with fallbacks of 1 and 5, and validation), and update the search params whenever page or per-page selection changes (e.g., always setting ?page=<currentPage>&perPage=<perPage>). Ensure that when perPage changes you reset to page 1 and immediately reflect page=1&perPage=<value> in the URL so a reload or bookmark restores the same view. Aside from the missing router behavior, your stateless Pagination component, active/disabled states, per-page selector, and info text are all implemented well, so once the URL syncing is in place the solution should fully 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread tsconfig.json
@@ -1,6 +1,17 @@
{
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using React Router to persist query params (?page=...&perPage=...) and apply them on page load. This file currently only imports React and useState. Import and use appropriate router hooks (for example, useSearchParams, useNavigate, or useLocation plus useEffect) so you can read params on mount and update them on changes. Also ensure the app is wrapped with a Router (e.g., BrowserRouter) at the root.

Comment thread tsconfig.json
Comment on lines +10 to +11
"resolveJsonModule": true,
"isolatedModules": true,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial currentPage and perPage are hardcoded. According to the requirements, you must read page and perPage from the URL query params (if present) on load and use them to initialize state. Consider parsing search params in an effect or deriving initial values from useSearchParams.

Comment thread src/App.tsx
@@ -1,25 +1,45 @@
import React from 'react';
import React, { useState } from 'react';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using React Router to persist and apply query params (e.g., ?page=2&perPage=7). There are no React Router imports or usage here. Import and use React Router hooks (for example, useSearchParams or useLocation/useNavigate) so the app can read query params on load and update them when page/perPage change.

Comment thread src/App.tsx
Comment on lines +10 to +11
const [currentPage, setCurrentPage] = useState<number>(1);
const [perPage, setPerPage] = useState<number>(5);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currentPage and perPage are initialized from hardcoded useState values. Per requirements, these should be initialized from URL query parameters when present (fallback to defaults 1 and 5). Parse the query params on mount and set initial state accordingly.

Comment thread src/App.tsx
Comment on lines +36 to +41
onChange={event => {
const newPerPage = Number(event.target.value);

setPerPage(newPerPage);
setCurrentPage(1);
}}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the perPage selector changes you reset to page 1 and update state, which is correct, but you also must update the URL query params so ?page=1&perPage= is reflected in the address bar. Update the search params here (via router) in addition to updating state.

Comment thread src/App.tsx
Comment on lines +56 to +60
<Pagination
total={total}
perPage={perPage}
currentPage={currentPage}
onPageChange={setCurrentPage}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pagination's onPageChange is wired to setCurrentPage directly. You should wrap this so that when the page changes you both update component state and persist the new ?page value in the URL (keeping perPage in sync). This ensures reload/bookmark restores the same pagination state as required.

@@ -1 +1,84 @@
export const Pagination = () => {};
import React from 'react';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using React Router to persist ?page=...&perPage=.... React Router hooks (e.g. useSearchParams or useLocation/useNavigate) are not imported here — you should import and use them to read/write query params.

Comment on lines +10 to +11
export const Pagination: React.FC<Props> = ({
total,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial currentPage and perPage should be derived from the URL query parameters on page load (e.g. parse ?page and ?perPage), not always defaulted to 1 and 5. Add logic to initialize state from the router query params or set them after mount.

Comment on lines +39 to +40
>
«
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When changing perPage you must also update the URL to reflect ?page=1&perPage=... so the selection is persisted. Currently this only updates component state (setPerPage / setCurrentPage(1)) but does not touch the router search params.

Comment on lines +56 to +61
onPageChange(page);
}
}}
>
{page}
</a>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the page changes via the Pagination component you must sync that change into the URL query params (so ?page=...&perPage=... stays current). Either wrap setCurrentPage to also update the query params here or use an effect that synchronizes currentPage/perPage to the router.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants