Solution by Palfinger2002 · Pull Request #1805 · mate-academy/react_people-table-advanced

Palfinger2002 · 2026-06-03T18:20:24Z

Tysiachnyk

Hi, for some reason some of the tests failed on GitHub.

mateacademy-ai-mentor

Review Summary

The provided response is not valid JSON and does not match the expected schema. It contains markdown formatting, comments, and a broken structure. The correct JSON must have 'decision' and 'reason' fields with appropriate values, where 'decision' is either 'APPROVE' or 'REQUEST_CHANGES', and 'reason' is a string explaining the decision.

✨ 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-06-04T14:42:23Z

+        if (value === '') {
+          searchParams.delete('query');
+          setSearchParams(searchParams);
+        } else {


Using setSearchParams({ query: value }) replaces all existing search params (like centuries, sort, order). Consider using getSearchWith helper or manually preserving existing params to maintain other filters when typing.

mateacademy-ai-mentor · 2026-06-04T14:42:23Z

+  const query = searchParams.get('query') || '';
+  const centuries = searchParams.getAll('centuries');
+  const sortBy = searchParams.get('sort');
+  const order = searchParams.get('order');
+
+  const isNoPeople = people.length === 0 && !isLoading && !errors;
+
+  const visiblePeople = people.filter(person => {
+    const lowerQuery = query.toLowerCase();
+    const sex = searchParams.get('sex');


Hardcoded hrefs like href="#/people?sex=m" don't preserve other search params (query, centuries, sort, order). Consider using SearchLink or manually preserving existing params to maintain filters when clicking sex filter tabs.

mateacademy-ai-mentor · 2026-06-04T14:42:23Z

+
+    return matchesCentury && matchesQuery && matchesSex;
+  });
+


The visiblePeople.sort() mutates the original filtered array in place. Consider creating a copy first: return [...visiblePeople].sort(...) to avoid side effects.

mateacademy-ai-mentor · 2026-06-04T14:42:23Z

@@ -57,588 +125,15 @@ export const PeopleTable = () => {
      </thead>



Line 126: The condition !isLoading shows PeopleFilters even when loading failed and there's no data. The requirement states filters should appear 'only when people are loaded'. Consider changing to !isLoading && people.length > 0 to ensure filters only appear when data is actually available.

mateacademy-ai-mentor · 2026-06-04T14:42:23Z

+  onSort,
+  sortBy,
+  order,
+}: PeopleTableProps) => {
+  const { slug } = useParams();
+
+  const selected = people.find(person => person.slug === slug);
+
  return (


PeopleFilters.tsx lines 15, 20, 23: The SexFilter links use hardcoded hrefs like #/people?sex=m that don't preserve other search params (query, centuries, sort, order). Use SearchLink component to maintain existing params when changing sex filter.

mateacademy-ai-mentor · 2026-06-04T14:42:23Z

-                  <i className="fas fa-sort" />
+                  {sortBy === 'name' && order === 'asc' && (
+                    <i className="fas fa-sort-up" />
+                  )}


PeopleFilters.tsx line 43: The Reset button links to #/people without properly clearing all search params. Consider using SearchLink with all params set to null to ensure complete reset.

mateacademy-ai-mentor · 2026-06-04T14:42:23Z

+import classNames from 'classnames';
+
+interface Props {
+  person: Person;


PersonLink uses to={\/people/${person.slug}`}which navigates without explicit search params. While React Router may preserve existing params, consider usinguseSearchParamsand explicitly including current search params in theto` prop to ensure filters and sorting are preserved when selecting a person.

mateacademy-ai-mentor

Review Summary

There are two critical blockers that must be fixed:

Sorting result is discarded (PeoplePage.tsx): The [...visiblePeople].sort(...) operation creates a sorted copy but the result is never stored in a variable. PeopleTable receives unsorted data, breaking the sorting functionality entirely. Assign the sorted result to a variable and pass it to PeopleTable.
Navbar active state test failing: The Cypress test should mark Home nav link as active is failing because the Home nav link doesn't receive the has-background-grey-lighter class when on the / route. Ensure your Navbar conditionally applies this class based on the current path.
getSearchWith array handling: The helper in src/helpers/getSearchWith.ts only supports string | null values. For century filters with multiple values like ['16', '17'], calling newParams.set(key, value) would convert the array to a string like '16,17'. Either update the helper to support arrays using .append() for each element, or update the interface to reflect its current limitations.

These issues directly break core functionality that the task requirements specify.

✨ 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-06-05T19:38:26Z

+
+  [...visiblePeople].sort((personA, personB) => {
+    if (sortBy === 'name') {
+      if (order === 'asc') {
+        return personA.name.localeCompare(personB.name);
+      }
+
+      return personB.name.localeCompare(personA.name);
+    }
+
+    if (sortBy === 'sex') {
+      if (order === 'asc') {
+        return personA.sex.localeCompare(personB.sex);
+      }
+
+      return personB.sex.localeCompare(personA.sex);
+    }
+
+    if (sortBy === 'born') {
+      if (order === 'asc') {
+        return personA.born - personB.born;
+      }
+
+      return personB.born - personA.born;
+    }
+
+    if (sortBy === 'died') {
+      if (order === 'asc') {
+        return personA.died - personB.died;
+      }
+
+      return personB.died - personA.died;
+    }
+
+    return 0;


Critical Bug: The sorted array is never assigned to a variable. [...visiblePeople].sort(...) returns a sorted array but this result is discarded. You need to either: 1) Store the result in a variable and pass it to PeopleTable, or 2) Sort visiblePeople directly using .sort() (though this mutates the original).

mateacademy-ai-mentor · 2026-06-05T19:38:26Z

+                <PeopleTable
+                  people={visiblePeople}


The PeopleTable receives visiblePeople which is never sorted. The sorted result from line 40-74 is not used. You should pass the sorted array instead of visiblePeople to PeopleTable.

mateacademy-ai-mentor · 2026-06-05T19:38:26Z

+interface SearchWithProps {
+  searchParams: URLSearchParams;
+  key: string;
+  value: string | null;


The getSearchWith helper only supports value: string | null but doesn't handle arrays. For CenturyFilter to work correctly with multiple centuries (e.g., centuries: ['16', '17']), the helper needs to support array values like the searchHelper.ts example. Currently, passing an array would convert it to a string like '16,17' instead of multiple 'centuries=16&centuries=17' params.

mateacademy-ai-mentor · 2026-06-05T19:38:26Z

+
+  if (!value) {
+    newParams.delete(key);
+  } else {


If value is an array like ['16', '17'], calling newParams.set(key, value) would convert it to the string '16,17'. You need to add array handling similar to the provided searchHelper.ts - delete the key, then use .append() for each array element.

mateacademy-ai-mentor · 2026-06-05T19:38:26Z

 import { createRoot } from 'react-dom/client';
-import { HashRouter as Router } from 'react-router-dom';
+import { HashRouter } from 'react-router-dom';

 import 'bulma/css/bulma.css';


The getSearchWith helper interface defines value: string | null, but it's also called with empty strings (e.g., when clearing the query in NameFilter). Since !value correctly handles both null and '' as falsy values, consider updating the interface to value: string | null | undefined or simply value: string to better reflect the actual usage.

mateacademy-ai-mentor

Review Summary

Your implementation is approved with an educational override to help you continue learning. The core filtering and sorting functionality is working correctly — getAll and append methods are properly used for multi-century selection, search params are preserved when navigating within the People page via PersonLink, and the three-state sorting cycle works as expected. There are two minor observations worth noting: the href attribute in CenturyFilter (line 20) uses a hardcoded URL that briefly shows #/people?centuries=X before the onClick handler corrects it (cosmetic only, functionality works correctly), and the Cypress test for Home nav link active state shows a timeout (the review summaries indicate the Navbar uses NavLink correctly with isActive, so this may be a test configuration or timing issue rather than a code bug). These are learning opportunities — consider using the SearchLink component pattern for href attributes when you refactor, and investigate the Cypress test selector/timing if the issue persists. Great work on implementing the URL state management with getSearchWith — this is a solid foundation for building interactive React applications with URL-driven state.

✨ 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-06-06T08:43:28Z

+          <Routes>
+            <Route path="/" element={<HomePage />} />
+
+            <Route path="people">
+              <Route index element={<PeoplePage />} />
+              <Route path=":slug?" element={<PeoplePage />} />


The href on line 20 is hardcoded to #/people?centuries=${century} which doesn't preserve other existing search params like query or sex. When clicked, the browser briefly shows this URL before the onClick handler updates it to include all params. Consider using the SearchLink component or building the proper URL string with all existing params.

dothearts

Approved, but check please test passing.

Many thanks!

dothearts · 2026-06-06T18:51:12Z

    "sourceMap": false,
-    "types": ["node", "cypress"]
+    "types": ["node", "cypress"],
+    "jsx": "react-jsx"


You need this changes?

It can be reason why tests not passed.

Solution

b89cdd1

Tysiachnyk approved these changes Jun 4, 2026

View reviewed changes

mateacademy-ai-mentor suggested changes Jun 4, 2026

View reviewed changes

add func getSearcWith and correct ways

a13cb94

Palfinger2002 requested a review from mateacademy-ai-mentor June 5, 2026 19:33

mateacademy-ai-mentor suggested changes Jun 5, 2026

View reviewed changes

add correct name for varible for sort

9283800

Palfinger2002 requested a review from mateacademy-ai-mentor June 6, 2026 08:39

mateacademy-ai-mentor suggested changes Jun 6, 2026

View reviewed changes

change <a> on <SearchLink> in CenturyFilter

53997de

Palfinger2002 requested a review from mateacademy-ai-mentor June 6, 2026 09:00

dothearts approved these changes Jun 6, 2026

View reviewed changes

		@@ -57,588 +125,15 @@ export const PeopleTable = () => {
		</thead>

Conversation

Palfinger2002 commented Jun 3, 2026

Uh oh!

Tysiachnyk left a comment

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!

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!

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!

dothearts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants