task solution by anastasiiadns · Pull Request #1820 · mate-academy/react_people-table-advanced

anastasiiadns · 2026-06-18T10:04:10Z

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are a couple of critical, requirement-breaking issues that affect core functionality.

Critical issues that must be fixed:

Search params are lost when selecting a person: PersonLink (and other person links such as in PeopleTable) use a plain NavLink like to={"/people/${person.slug}"} without preserving the current location.search. The task explicitly requires keeping all search params (filters and sorting) when navigating within the People page; currently, clicking a person clears query, sex, centuries, sort, and order. You should adjust these links to include the existing search string (e.g., to={{ pathname: "/people/${person.slug}", search: location.search }} or via your SearchLink helper) so navigation to /people/:slug keeps filters and sort.
Potential runtime error in name filtering: In PeoplePage.tsx, the query filter uses person.motherName?.toLowerCase().includes(query) and similarly for fatherName. When motherName or fatherName is null, the optional chaining returns undefined, and calling .includes on undefined will throw at runtime, breaking the page for some records. Coerce these fields to empty strings or split the checks, for example: (person.motherName || '').toLowerCase().includes(query) and the same for fatherName.

Minor but non-blocking suggestions:

The navbar People link should also preserve search params so filters/sort remain when navigating back to the list, using the same search-preserving approach as above.
Consider validating that sort matches allowed fields before casting to SortField, and remove the unused FilterPerson enum to keep the codebase clean.

Your URL-based filtering and sorting logic, multi-century handling, and three-step sort cycle are all well implemented; once you fix the search-param preservation for person links and the null-safe name filtering, your solution will fully align with the task requirements.

✨ 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-18T10:06:09Z

+  const allCenturies = ['16', '17', '18', '19', '20'];
+


PersonLink uses a plain NavLink to /people/${person.slug} and does not preserve existing search params, so filters and sorting are lost when selecting a person. The requirements state that search params must be kept when navigating within the People page; consider using SearchLink or otherwise including the current location.search in the destination URL.

mateacademy-ai-mentor · 2026-06-18T10:06:09Z

+      params.set('query', value);
+    } else {
+      params.delete('query');
+    }
+
+    setSearchParams(params);
+  }
+
+  function getToggledCenturies(century: string) {
+    return centuries.includes(century)


getNextSort correctly implements the three-step sort cycle (asc, desc, off) and uses null to signal removal of sort/order params, matching the URL param requirements for sorting. No changes needed here, but keep in mind this relies on getSearchWith to delete null keys.

mateacademy-ai-mentor · 2026-06-18T10:06:09Z

+  function handleQueryChange(event: React.ChangeEvent<HTMLInputElement>) {
+    const params = new URLSearchParams(searchParams);
+    const value = event.target.value;
+
+    if (value) {
+      params.set('query', value);
+    } else {
+      params.delete('query');
+    }
+
+    setSearchParams(params);


The name filter handler correctly updates the query search param and removes it when the input is empty, as required. Just ensure you don’t accidentally bypass SearchLink/getSearchWith elsewhere, since URL param logic is centralized there.

mateacademy-ai-mentor · 2026-06-18T10:06:10Z

+      if (!sortField) {
+        return 0;
+      }
+
+      const valueA = a[sortField];


Sorting relies on sortField being a valid key of Person, but sort is just a string from the URL; if it’s something unexpected, a[sortField] can become undefined. Consider validating that sort matches one of the allowed fields before casting to SortField to keep the logic robust.

mateacademy-ai-mentor · 2026-06-18T10:06:10Z

+  const [loading, setLoading] = useState(true);
+  const [hasError, setHasError] = useState(false);


This link navigates to /people/${person.slug} without preserving the existing search params (filters and sort). The requirements state that search params must be kept when selecting a person on the People page, so you should adjust this to include the current search string or use your SearchLink-based approach.

mateacademy-ai-mentor · 2026-06-18T10:06:10Z

+
+export const PeopleTable: React.FC<Props> = ({ people }) => {


Using a plain NavLink here loses the current search params (filters/sorting) when navigating to a person, which violates the requirement to keep search params within the People page. Consider using SearchLink or otherwise preserving location.search when building the to value.

mateacademy-ai-mentor · 2026-06-18T10:06:10Z

+    <NavLink
+      to={`/people/${person.slug}`}
+      className={classNames({
+        'has-text-danger': person.sex === 'f',
+      })}
+    >
+      {person.name}
+    </NavLink>


PersonLink uses a plain NavLink to /people/${person.slug} without preserving location.search or using SearchLink. This violates the requirement to "keep search params when navigating within the People page (when selecting a person)" because clicking on a person clears active filters/sorting. Consider obtaining current search params (e.g. via useSearchParams or useLocation) and including them in the to prop, or rewriting this to use SearchLink so the query, centuries, sex, sort, and order remain in the URL.

mateacademy-ai-mentor · 2026-06-18T10:06:10Z

+export enum FilterPerson {
+  All = 'all',
+  Male = 'male',
+  Female = 'female',


This enum is currently not used anywhere in your implementation. Unused types can be removed to keep the codebase clean, or, if you intend to use it, integrate it into your filtering logic so the values align with your sex filter approach.

mateacademy-ai-mentor · 2026-06-18T10:06:10Z

+export enum FilterPerson {
+  All = 'all',
+  Male = 'male',
+  Female = 'female',
+}


This enum correctly enumerates the fields allowed for sorting and matches the task requirement to sort by name, sex, born, and died. Consider using these enum members instead of raw strings where possible to avoid typos in sort field names.

mateacademy-ai-mentor · 2026-06-18T10:06:10Z

+export enum SortField {
+  Name = 'name',
+  Sex = 'sex',
+  Born = 'born',


This enum isn’t referenced anywhere in the provided code, so it adds unnecessary noise. Either integrate it into your filtering logic (replacing raw sex strings) or remove it to avoid confusion.

mateacademy-ai-mentor

Review Summary

I am approving your solution because it fully meets the functional requirements for filtering, sorting, and URL-based state management on the People page. All filters (query, sex, centuries) and sorting (sort, order) are correctly stored in and read from URL search params, and you preserve those params when navigating via the navbar and person links, so state is shareable and stable across views. The name filter properly handles name, motherName, and fatherName in a case-insensitive, null-safe way, and the century filter correctly supports multi-select using repeated centuries params with getAll/append. Your three-step sorting cycle (asc → desc → off) is implemented as required and correctly removes sort and order from the URL when sorting is disabled. The filters sidebar only appears once people are successfully loaded, and error/empty states are handled cleanly.

The remaining points are minor and non-blocking: you mix setSearchParams and SearchLink for updating filters, which works correctly but could be unified for stylistic consistency, and you could optionally validate sort against the SortField enum for extra robustness against unexpected URLs. Overall, this is a solid, well-structured implementation that satisfies the task’s core expectations—nice work.

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

Rate AI review example ↩

task solution

b3844db

mateacademy-ai-mentor suggested changes Jun 18, 2026

View reviewed changes

fix of task

9593052

anastasiiadns requested a review from mateacademy-ai-mentor June 18, 2026 10:19

mateacademy-ai-mentor approved these changes Jun 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

task solution#1820

task solution#1820
anastasiiadns wants to merge 2 commits into
mate-academy:masterfrom
anastasiiadns:develop

anastasiiadns commented Jun 18, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor Jun 18, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		const [loading, setLoading] = useState(true);
		const [hasError, setHasError] = useState(false);


		export const PeopleTable: React.FC<Props> = ({ people }) => {

Uh oh!

Conversation

anastasiiadns commented Jun 18, 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