completed the task by SerhiyShimko · Pull Request #1477 · mate-academy/react_pagination

SerhiyShimko · 2026-04-17T16:02:28Z

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still functional issues that break the required behavior of the pagination component and violate several checklist items.

Critical issues that must be fixed:

In both App.tsx and src/components/Pagination/Pagination.tsx, the items for the current page are derived using a 1-based startsWith and splice, which causes an off‑by‑one error and mutates the array; instead, compute a zero‑based startIndex = (currentPage - 1) * perPage and render items.slice(startIndex, startIndex + perPage) so that the visible items line up with the info text [CHECKLIST ITEM #1, #10].
The page-number list item click handler currently always calls onPageChange(button) even when button === currentPage; you need to guard this and only call the callback when the page actually changes (e.g., if (button !== currentPage) onPageChange(button)), including for the numbered buttons [CHECKLIST ITEM #4].
The currentPage prop in Pagination.tsx is declared as required and is destructured without a default; the task requires it to be optional (currentPage?: number) with a default of 1 in the parameter destructuring so the component works when currentPage is omitted [CHECKLIST ITEM #16].
The per‑page <select> in both Pagination.tsx and App.tsx is uncontrolled because it relies on selected on an <option>; in React you should control it via value={perPage} on the <select> and remove selected from the options so the UI reliably reflects state [CHECKLIST ITEM #11].

Minor improvements (not blockers, but worth addressing): use the item number (or text) rather than the index as the React key for page buttons, and consider preventing default navigation on the page links or moving the onClick handler onto the <a> to avoid hash changes. Overall, the structure is solid and you are close; fixing these issues will bring your solution in line with the required behavior and make the pagination fully consistent and testable.

✨ 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-17T16:04:44Z

+            }
+          }}
+        >
+          «
+        </a>
+      </li>
+      {buttons.map((button, index) => {
+        return (
+          <li
+            onClick={() => {
+              onPageChange(button);


The is not controlled — you should set the select's value to the current perPage and remove the selected attribute on the . In React you should control the selection via value={perPage} on the instead of using selected on an option.

mateacademy-ai-mentor · 2026-04-17T16:04:44Z

+};
+
+export const Pagination: React.FC<Props> = ({
+  total,


The startsWith value is a 1-based (human) position which is correct for the info text, but array indexing is zero-based. Compute a zero-based start index (e.g. const startIndex = (currentPage - 1) * perPage) to use when slicing the items array.

mateacademy-ai-mentor · 2026-04-17T16:04:44Z

+          className="page-link"
+          href="#next"
+          aria-disabled={currentPage === buttons.length ? 'true' : 'false'}
+          onClick={() => {


You use [...items].splice(startsWith, perPage) which (a) uses a 1-based startsWith leading to an off-by-one and (b) uses splice (mutating) instead of slice. Replace with something like items.slice(startIndex, startIndex + perPage) where startIndex = (currentPage - 1) * perPage so the rendered items match the info text.

mateacademy-ai-mentor · 2026-04-17T16:04:44Z

+type Props = {
+  total: number;
+  perPage: number;
+  currentPage: number;
+  onPageChange: (page: number) => void;
+};


The currentPage prop is declared as required in the Props type. The task requires currentPage to be optional with a default of 1 — change the type to currentPage?: number.

mateacademy-ai-mentor · 2026-04-17T16:04:44Z

+export const Pagination: React.FC<Props> = ({
+  total,
+  perPage,
+  currentPage,
+  onPageChange,
+}) => {


Provide a default value for currentPage when destructuring props so the component behaves correctly when currentPage isn't provided: e.g. currentPage = 1 in the parameter list.

mateacademy-ai-mentor · 2026-04-17T16:04:44Z


 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-const items = getNumbers(1, 42).map(n => `Item ${n}`);
+const items: string[] = getNumbers(0, 42).map(n => `Item ${n}`);


The items array is built with getNumbers(0, 42) which likely produces Item 0 as the first element. Your pagination calculations (startsWith) are 1-based, so this can cause an off-by-one mismatch between the displayed info and the actual items. Consider creating items starting from 1 (e.g. getNumbers(1, 42)) or adjust indices consistently.

mateacademy-ai-mentor · 2026-04-17T16:04:44Z

+          >
            <option value="3">3</option>
-            <option value="5">5</option>
+            <option value="5" selected>


Using selected on an <option> in React is discouraged and makes the select uncontrolled. Instead, make the <select> controlled by providing value={perPage} on the <select> and remove selected from the option elements. This ensures the UI reflects the perPage state.

mateacademy-ai-mentor · 2026-04-17T16:04:44Z

-        <li data-cy="item">Item 3</li>
-        <li data-cy="item">Item 4</li>
-        <li data-cy="item">Item 5</li>
+        {[...items].splice(startsWith, perPage).map((item, index) => {


This slice uses splice(startsWith, perPage). startsWith is 1-based (page item number), but splice uses a 0-based index so you should use startsWith - 1. Also splice mutates the array copy; prefer slice(startIndex, endIndex) e.g. items.slice(startsWith - 1, startsWith - 1 + perPage) to return the correct items for the current page without mutation. Fixes checklist item #1 (show only items for current page).

mateacademy-ai-mentor · 2026-04-17T16:04:44Z

+  const [currentPage, changeCurrentPage] = useState(1);
+  const startsWith = perPage * currentPage - (perPage - 1);
+  const total = 42;
+  let ends = perPage * currentPage;
+
+  if (ends > total) {


The component currently requires currentPage in the Props destructuring. Per requirement #16, currentPage should be optional with a default of 1. Provide a default value in the destructuring, for example: ({ total, perPage, currentPage = 1, onPageChange }) so the component works when currentPage is omitted.

mateacademy-ai-mentor · 2026-04-17T16:04:44Z

+          >
            <option value="3">3</option>
-            <option value="5">5</option>
+            <option value="5" selected>
+              5


Clicking a page number always calls onPageChange(button) even when button === currentPage. Per requirement #4, only trigger onPageChange if the page actually changes. Add a guard: if (button !== currentPage) onPageChange(button) to avoid unnecessary callbacks.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still a couple of critical functional requirements missing or incorrect, which means the pagination behavior does not yet match the task specification. In src/App.tsx, the current-page items are sliced using a 1-based startIndex ((currentPage - 1) * perPage + 1), but the task requires a zero-based index: change this to const startIndex = (currentPage - 1) * perPage; and slice with items.slice(startIndex, startIndex + perPage) so the visible items align correctly with the info text and tests [CHECKLIST ITEM #1, #10]. After you make startIndex zero-based in both App.tsx and Pagination.tsx, compute a separate displayStart = startIndex + 1 and use displayStart in the info paragraph so it shows human-friendly numbers like Page 1 (items 1 - 5 of 42) instead of starting from 0. A second blocker is that React Router integration for query params is missing: you need to read page and perPage from the URL on initial load (e.g. via useSearchParams or useLocation/useNavigate) and update the URL (?page=2&perPage=7) whenever currentPage or perPage changes, so pagination state is persisted and restorable.

Additionally, there are a few smaller but still important adjustments: in Pagination.tsx the per-page selector should be rendered and controlled via props such as perPage and onPerPageChange, and your <a> elements for prev/next and numbered pages should call event.preventDefault() (or move the handler to the anchor) so clicking them doesn’t change the URL hash. As a minor quality improvement, replace key={index} with a stable key like key={item} for the items list and page buttons to avoid potential rendering issues. Once you correct the off-by-one indexing, add router-based query syncing, wire the per-page selector into Pagination, and prevent default link navigation, your solution will align well with the checklist and your existing structure will then be in great 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.

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

+  total,
+  perPage,


The startIndex is computed as (currentPage - 1) * perPage + 1, which is a 1-based index. The task requires a zero-based start index: startIndex = (currentPage - 1) * perPage, and then use items.slice(startIndex, startIndex + perPage) so visible items line up with the info text. After that, compute a display start for the UI: displayStart = startIndex + 1.

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

+          data-cy="prevLink"
+          className="page-link"
+          href="#prev"
+          aria-disabled={currentPage === 1 ? 'true' : 'false'}


The info paragraph uses startIndex for display. If you change startIndex to be zero-based (as required), update the displayed start to startIndex + 1 (e.g. use displayStart) so the text shows Page 1 (items 1 - 5 of 42).

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

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


The app currently has no React Router usage. The task requires saving ?page=2&perPage=7 into the URL and applying them on page load. Add router hooks (for example useLocation/useHistory in v5 or useSearchParams in v6) to read initial page and perPage from query params and to push/replace the query params when currentPage or perPage change.

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

+      {buttons.map(button => {
+        return (
+          <li
+            onClick={() => {
+              if (button !== currentPage) {
+                onPageChange(button);
+              }
+            }}
+            className={
+              currentPage === button ? 'page-item active' : 'page-item'
+            }
+            key={button}
+          >
+            <a data-cy="pageLink" className="page-link" href={`#${button}`}>
+              {button}
+            </a>


In the Pagination component the numbered anchors use href="#..." and the click handler for changing page is on the li. Clicking the <a> will still change the URL hash. Prevent default navigation (call e.preventDefault()), or move the onClick to the <a> and prevent default there to avoid unwanted hash changes.

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

+          href="#next"
+          aria-disabled={currentPage === buttons.length ? 'true' : 'false'}
+          onClick={() => {
+            if (currentPage !== buttons.length) {


Using the array index as the key for list items can cause rendering issues. Use the item text or number (e.g. key={item}) which is unique, instead of key={index}.

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

 import { getNumbers } from './utils';

 // eslint-disable-next-line @typescript-eslint/no-unused-vars
-const items = getNumbers(1, 42).map(n => `Item ${n}`);
+const items: string[] = getNumbers(0, 42).map(n => `Item ${n}`);



The per-page <select> is present and controlled in App (good), but the checklist requires the per-page selector to be a controlled component in both Pagination.tsx and App.tsx. Pagination.tsx does not render the <select> and its Props type doesn't accept a handler to change perPage. Add a prop like onPerPageChange: (perPage: number) => void and render a controlled <select data-cy="perPageSelector" value={perPage} ...> inside Pagination.

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

+            onChange={e => {
+              changeCurrentPage(1);
+              changePerPage(+e.target.value);
+            }}


The numbered page onClick handler is attached to the li while the a anchors still have hrefs that change the hash. Prefer moving the click handler to the <a> and calling event.preventDefault() so clicks don't change the URL hash. Also keep the guard that only calls onPageChange when the page actually changes (you already have if (button !== currentPage) — keep that).

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

      <p className="lead" data-cy="info">
-        Page 1 (items 1 - 5 of 42)
+        Page {currentPage} (items {startIndex} - {ends} of {total})
      </p>


Similarly, the prev link's onClick does not call preventDefault(), so clicking will change the hash. Update the handler to accept the click event and call event.preventDefault() before running the onPageChange guard. Same for the next link.

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

-        <li data-cy="item">Item 5</li>
+        {nowItems.map((item, index) => {
+          return (
+            <li data-cy="item" key={index}>


The next link also should prevent default navigation to avoid hash changes. Update the onClick handler on the next anchor to receive the event and call event.preventDefault() before calling onPageChange (when appropriate).

mateacademy-ai-mentor · 2026-04-17T16:21:23Z

-        <li data-cy="item">Item 5</li>
+        {nowItems.map((item, index) => {
+          return (
+            <li data-cy="item" key={index}>


Minor: when rendering the items list in App, prefer using a stable key such as the item string instead of the array index to avoid potential key collisions and to satisfy the low-priority checklist suggestion.

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override so you can keep progressing while you address a few remaining issues. You did a solid job making Pagination stateless, wiring onPageChange, handling disabled prev/next buttons, and including the required data-cy attributes and info text format. The most critical fix is in src/App.tsx (and similarly in src/components/Pagination/Pagination.tsx): your slice uses startIndex + 1, but the requirement is zero-based slicing, so update it to items.slice(startIndex, startIndex + perPage) and derive a separate displayStart = startIndex + 1 for the human-readable info [CHECKLIST ITEM #1]. Another key requirement is to move the per-page <select data-cy="perPageSelector"> into the Pagination component and control it via perPage and an onPerPageChange prop, instead of rendering it in App.tsx [CHECKLIST ITEM #4]. You also need to integrate React Router query params so page and perPage are read from the URL on load and the URL is updated whenever they change (e.g. via useSearchParams), which persists pagination state across reloads [CHECKLIST ITEM #10]. Finally, make sure every pagination <a> (including numbered page links) either moves the click handler onto the anchor and calls event.preventDefault() or removes the href so clicking a page link does not change the URL hash. Overall, your structure and logic are close—tightening these details will bring your implementation fully in line with the task requirements and is a great learning step for real-world pagination and routing.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-04-17T16:33:29Z

+  const [perPage, changePerPage] = useState(5);
+  const [currentPage, changeCurrentPage] = useState(1);
+  const startIndex = (currentPage - 1) * perPage;
+  const nowItems = items.slice(startIndex + 1, startIndex + 1 + perPage);


The slice is off-by-one. The code uses startIndex + 1 but the task requires zero-based slicing. Change to use items.slice(startIndex, startIndex + perPage) so the shown items match the info text and tests.

mateacademy-ai-mentor · 2026-04-17T16:33:30Z


      <p className="lead" data-cy="info">
-        Page 1 (items 1 - 5 of 42)
+        Page {currentPage} (items {startIndex + 1} - {ends} of {total})


After fixing zero-based slicing you should compute a human-friendly start index (e.g. const displayStart = startIndex + 1) and use that in the info paragraph instead of using startIndex + 1 inline. This keeps logic clear and matches the requirement Page 1 (items 1 - 5 of 42).

mateacademy-ai-mentor · 2026-04-17T16:33:30Z

          <select
+            value={perPage}
            data-cy="perPageSelector"
            id="perPageSelector"
-            className="form-control">
+            className="form-control"
+            onChange={e => {
+              changeCurrentPage(1);
+              changePerPage(+e.target.value);
+            }}
+          >
            <option value="3">3</option>
-            <option value="5">5</option>
+            <option value="5"> 5 </option>
            <option value="10">10</option>
            <option value="20">20</option>
          </select>


The per-page is rendered here in App, but the task requires the per-page selector to be rendered inside the stateless Pagination component and controlled via props (e.g. pass perPage and onPerPageChange). Move this selector into Pagination and remove it from App.

mateacademy-ai-mentor · 2026-04-17T16:33:30Z

+  const [perPage, changePerPage] = useState(5);
+  const [currentPage, changeCurrentPage] = useState(1);


Pagination state must be persisted in the URL using React Router query params. Initialize page and perPage from the URL on load (useSearchParams or useLocation) and update the query string whenever they change. Currently state is local only, so add router-based syncing here when initializing the states.

mateacademy-ai-mentor · 2026-04-17T16:33:30Z

+      <Pagination
+        total={total}
+        perPage={perPage}
+        currentPage={currentPage}
+        onPageChange={page => {
+          changeCurrentPage(page);
+        }}
+      />


When using the Pagination component you should also pass an onPerPageChange prop (after you move the selector into Pagination). Right now Pagination receives perPage/currentPage/onPageChange only; add and pass onPerPageChange from App so App can update perPage (and reset to page 1).

mateacademy-ai-mentor · 2026-04-17T16:33:30Z

+            }
+          }}
+        >
+          «
+        </a>
+      </li>
+      {buttons.map(button => {
+        return (
+          <li


The per-page selector must be rendered inside the stateless Pagination component and controlled via props (e.g. pass an onPerPageChange handler). Currently the lives in App; move it into Pagination and drive it via props so Pagination owns the UI for per-page selection as required.

mateacademy-ai-mentor · 2026-04-17T16:33:30Z

+};
+
+export const Pagination: React.FC<Props> = ({


React Router query param syncing is missing. The App should read page and perPage from the URL on initial load and update the URL ?page=...&perPage=... whenever currentPage or perPage changes so pagination state is persisted and restorable.

mateacademy-ai-mentor · 2026-04-17T16:33:30Z

+      </li>
+      {buttons.map(button => {
+        return (


When perPage changes you reset the current page to 1 (good), but you also need to update the URL query string (page=1&perPage=...) so the new state is persisted. Integrate this into the onChange handler (or centralize URL updates in App's state effects).

mateacademy-ai-mentor · 2026-04-17T16:33:30Z

+  total: number;
+  perPage: number;
+  currentPage?: number;
+  onPageChange: (page: number) => void;


Pagination props are missing an onPerPageChange (or similar) to allow App to control per-page value from the selector if you move it into Pagination. Add an onPerPageChange: (perPage: number) => void prop and render the inside Pagination as required.

mateacademy-ai-mentor · 2026-04-17T16:33:30Z

+            }
+            key={button}
+          >
+            <a data-cy="pageLink" className="page-link" href={`#${button}`}>


Numbered page anchors (<a data-cy="pageLink" ...>) currently have their click handler on the parent
and the anchor has an href that updates the hash. This violates the requirement to prevent default navigation. Move the click handler to the anchor (or call event.preventDefault() inside it) so clicks do not change the URL hash.

Denys-Kravchuk9988 · 2026-04-17T17:40:28Z

+
+  return (
+    <ul className="pagination">
+      <li className={currentPage === 1 ? 'page-item disabled' : 'page-item'}>


It's better to use classNames here

Denys-Kravchuk9988 · 2026-04-17T17:40:41Z

+              }
+            }}
+            className={
+              currentPage === button ? 'page-item active' : 'page-item'


It's better to use classNames here

Denys-Kravchuk9988 · 2026-04-17T17:40:51Z

+      })}
+      <li
+        className={
+          currentPage === buttons.length ? 'page-item disabled' : 'page-item'


It's better to use classNames here

Denys-Kravchuk9988 · 2026-04-17T17:44:09Z

+          className="page-link"
+          href="#prev"
+          aria-disabled={currentPage === 1 ? 'true' : 'false'}
+          onClick={e => {


It's better to extract this code into a separate callback and insert this callback here

completed the task

57f8cda

mateacademy-ai-mentor suggested changes Apr 17, 2026

View reviewed changes

completed the task

bc13b06

SerhiyShimko requested a review from mateacademy-ai-mentor April 17, 2026 16:18

mateacademy-ai-mentor suggested changes Apr 17, 2026

View reviewed changes

completed the task

8a35c74

SerhiyShimko requested a review from mateacademy-ai-mentor April 17, 2026 16:30

mateacademy-ai-mentor approved these changes Apr 17, 2026

View reviewed changes

Denys-Kravchuk9988 suggested changes Apr 17, 2026

View reviewed changes

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

		const [perPage, changePerPage] = useState(5);
		const [currentPage, changeCurrentPage] = useState(1);

Conversation

SerhiyShimko 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!