solution, But I think I made some mistakes. by Ivadriil · Pull Request #1823 · mate-academy/react_people-table-advanced

Ivadriil · 2026-06-19T19:18:33Z

mateacademy-ai-mentor

Review Summary

I am requesting changes because a few core checklist requirements are still not met, and they will likely cause tests and required behaviors to fail.

Critical issues that must be fixed before this can pass:

query param must be removed when the input is empty ([CHECKLIST ITEM #8, #18]). In PeopleFilters.tsx, PeoplePage.tsx, and PeopleTable.tsx you always do params.set('query', event.target.value), so when the input is cleared the URL becomes ?query= instead of having no query key. Update the change handlers so that when event.target.value.trim() is empty you call params.delete('query'), and only set('query', value) when it is non‑empty.
Search params must be preserved when navigating within the People page ([CHECKLIST ITEM #4, #16]). The Navbar People link (src/components/Navbar.tsx) uses to="/people", and PersonLink (src/components/PersonLink.tsx) uses to={/people/${person.slug}}, both of which drop existing ?query=...&centuries=...&sort=...&order=.... Replace these with your SearchLink helper (or build a to object that keeps the current location.search) so that filters and sort remain in the URL when clicking People or a specific person.

Minor/structural suggestions (not blockers, but worth improving):

You currently have both PeopleFilters and PeopleTable calling onFilter, which makes the visible list depend on two sources (URL params and internal state). Consider centralizing filtering/sorting in one place derived solely from peoples + searchParams to keep the URL as the single source of truth.

Your implementation of the filters, multi-century support, and sorting logic is otherwise very solid; once you fix the query clearing and navigation search-param preservation, it should align well 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-19T19:20:31Z

+                'has-background-grey-lighter': isActive,
+              });
+            }}
+            to="/people"


According to checklist items #4 and #16, when the user clicks the People navigation, existing search parameters must be preserved; using a plain to="/people" will drop any current ?query=...&centuries=... filters. Consider using your SearchLink wrapper (or useSearchParams with a composed to) here so the search string is carried over.

mateacademy-ai-mentor · 2026-06-19T19:20:31Z

+  function hangeChangeQuery(event: React.ChangeEvent<HTMLInputElement>) {
+    const params = new URLSearchParams(serchParams);
+
+    params.set('query', event.target.value);
+    setSerchParams(params);


Here you always set the query param, even when the input is empty; the requirements (checklist #8/#18) say that when the input is empty there should be no query in the search params. Consider deleting query from params if event.target.value.trim() is an empty string instead of setting it.

mateacademy-ai-mentor · 2026-06-19T19:20:31Z

+  useEffect(() => {
+    let filtered = [...peoples];
+
+    if (centuries.length > 0) {
+      filtered = filtered.filter(person => {
+        const century = Math.ceil(person.born / 100);
+
+        return centuries.includes(String(century));
+      });
+    }
+
+    if (sex) {
+      filtered = filtered.filter(person => person.sex === sex);
+    }
+
+    if (query) {
+      filtered = filtered.filter(
+        person =>
+          person.name.toLowerCase().includes(query.trim().toLowerCase()) ||
+          person.fatherName
+            ?.toLowerCase()
+            .includes(query.trim().toLowerCase()) ||
+          person.motherName?.toLowerCase().includes(query.trim().toLowerCase()),
+      );
+    }
+
+    onFilter(filtered);
+  }, [peoples, serchParams, sex, query, onFilter]);


This effect recalculates filtered based on peoples and search params and then calls onFilter(filtered), while PeopleTable also calls onFilter when sorting. That double responsibility makes the visible list depend on both URL params and internal state, which conflicts with the requirement that filters/sort be controlled by URL search params. Consider centralizing filtering/sorting logic so that it derives solely from peoples + search params in one place.

mateacademy-ai-mentor · 2026-06-19T19:20:31Z

+
+    if (n === 'All') {
+      params.delete('centuries');
+
+      return setSerchParams(params);
+    }
+
+    const newCenturies = centuries.includes(n)
+      ? centuries.filter(centurie => centurie !== n)
+      : [...centuries, n];
+
+    params.delete('centuries');
+    newCenturies.forEach(ages => params.append('centuries', ages));
+
+    return setSerchParams(params);
+  }
+
+  const handleClearAll = () => {
+    setSerchParams({});
+  };
+
+  useEffect(() => {
+    let filtered = [...peoples];
+
+    if (centuries.length > 0) {
+      filtered = filtered.filter(person => {


sortPeople both updates local masPeople and calls onFilter(sorted), which then changes visiblePeoples in the parent. Since sorted is already stored locally in masPeople and the sort criteria are in the URL, propagating the sorted list upwards is redundant and can make state harder to reason about. Consider relying on URL params plus local state here, or centralizing the sort in the parent, but avoid two independent sorted states.

mateacademy-ai-mentor · 2026-06-19T19:20:31Z

+      .then(peopleFromServer => {
+        setPeoples(peopleFromServer);
+        setVisiblePeoples(peopleFromServer);
+        if (!peopleFromServer || peopleFromServer.length === 0) {
+          setChekError(TypeErroros.noPeople);


According to checklist items #6, #17, and especially #8/#18, when the search input is empty the query param should be removed from the URL, not set to an empty string. Here you always do params.set('query', event.target.value), so query= remains in the URL. Consider checking for an empty trimmed value and calling params.delete('query') when it’s empty, only set when it’s non-empty.

mateacademy-ai-mentor · 2026-06-19T19:20:32Z

+      if (typeof valA === 'number' && typeof valB === 'number') {
+        return order !== null ? valB - valA : valA - valB;


Calling setSerchParams({}) here clears all search params, which is good for a full reset. Just be aware that this also clears sort / order if they’re set, which is consistent with a “Reset all filters” button but goes beyond only filters; confirm this matches the desired UX.

mateacademy-ai-mentor · 2026-06-19T19:20:32Z

+      return 0;
+    });
+
+    setMasPeople(sorted);
+    onFilter(sorted);
+  }
+
+  function hangelSort(sor: string) {
+    const params = new URLSearchParams(serchParams);
+
+    if (currentSort === sor && currentOrder === 'desc') {
+      params.delete('sort');
+      params.delete('order');
+    } else if (currentSort === sor) {
+      params.set('order', 'desc');
+    } else {
+      params.set('sort', sor);
+      params.delete('order');
+    }
+
+    currentSort = params.get('sort') as keyof Person | null;
+    currentOrder = params.get('order') as 'desc' | null;
+    setSerchParams(params);
+    sortPeople(currentSort, currentOrder);
+
+    return;
+  }
+


This effect correctly applies all filters based on URL params, but using serchParams as a dependency means the effect will re-run on any param change (including unrelated ones). That’s generally fine here, but if you later add extra search params not related to filtering, you might want to derive only the needed values instead of depending on the whole serchParams object.

mateacademy-ai-mentor · 2026-06-19T19:20:32Z

+  function sortPeople(field: keyof Person | null, order: 'desc' | null) {
+    const sorted = [...peoples];
+
+    if (field === null) {
+      return setMasPeople([...peoples]);
+    }
+
+    sorted.sort((a, b) => {
+      const valA = a[field];
+      const valB = b[field];
+
+      if (typeof valA === 'string' && typeof valB === 'string') {
+        return order !== null
+          ? valB.localeCompare(valA)
+          : valA.localeCompare(valB);
+      }
+
+      if (typeof valA === 'number' && typeof valB === 'number') {
+        return order !== null ? valB - valA : valA - valB;
+      }
+
+      return 0;
+    });
+
+    setMasPeople(sorted);
+    onFilter(sorted);


sortPeople derives the sorted list from the peoples prop (the parent’s visible array) and then calls onFilter(sorted). This makes sorting update the parent’s filtered state. While not forbidden, it couples sorting and filtering: you might prefer to leave onFilter responsible for filtering only and keep sorted state local to this table, especially since sort state is already stored in search params.

mateacademy-ai-mentor · 2026-06-19T19:20:32Z

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


This Link navigates to /people/${person.slug} without preserving any existing search params (like ?query=...&centuries=...&sort=...). Checklist items #4 and #16 require keeping search params when navigating within the People page (including when selecting a person). Consider replacing this with your SearchLink component or building a to object that keeps the current location.search.

mateacademy-ai-mentor · 2026-06-19T19:20:32Z


 /**
- * SearchLink updates the given `params` in the search keeping the `pathname`
- * and the other existing search params (see `getSearchWith`)
+ * SearchLink оновлює задані `params` у пошуку, зберігаючи `pathname`


The requirement says that search params must be preserved when navigating within the People page, including when selecting a person (checklist #4, #16). Here you use a plain to={/people/${person.slug}}, which drops existing ?query, centuries, sort, and order from the URL. Consider replacing this with your SearchLink component or building a to object that reuses the current location.search so these params are kept.

mateacademy-ai-mentor

Review Summary

I am requesting changes because one core requirement is still not satisfied: search params are not preserved when clicking the People link in the navbar, which is explicitly required for correct behavior of the People page. Currently, src/components/Navbar.tsx uses a plain NavLink with to="/people", so any active filters or sorting in the URL (like ?query=&centuries=&sort=&order=) are lost when the user navigates via that link, breaking the shareable/filter-preserving navigation requirement.

The critical issue to fix is:

In Navbar.tsx, update the People navigation item to preserve the current search string when going to /people. For example, you can either use useLocation and set to={{ pathname: '/people', search: location.search }} for that link, or reuse your existing SearchLink helper with to="/people" and params={{}} so it keeps all current search params intact.

All other major requirements (URL-backed filters, multi-century selection, case-insensitive name/mother/father search, and the 3-click sort behavior with correct sort/order handling) are implemented well, and your PersonLink already preserves search params when selecting a person. Once you adjust the navbar People link to keep the search params, your solution will align fully with the task description. You’re very close; this is a small but important change to complete the functionality.

✨ 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-29T09:54:46Z

+                'has-background-grey-lighter': isActive,
+              });
+            }}
+            to="/people"


The People NavLink navigates to /people without preserving the current location.search, so any active filters (query, centuries, sort, order, etc.) are dropped when clicking this link. The description explicitly requires "Keep search params when navigating within the People page", and this was also flagged in the previous review. Consider switching this to a link that includes the existing search (e.g., using useLocation with a to={{ pathname: '/people', search: location.search }} or your SearchLink helper) so that URL search params remain intact.

mateacademy-ai-mentor

Review Summary

I am approving your solution because the People page behavior and URL-backed filters/sorting are implemented correctly and match the checklist requirements (name/century filters, multi-century selection, 3-click sorting, and search-param preservation on navigation like [CHECKLIST ITEM #1–#3, #9–#27, #31]). Your use of useSearchParams to drive both filters and sorting is solid, and the conditional rendering of the sidebar only when people are successfully loaded shows good attention to UX and state management. Navigation via Navbar and PersonLink correctly keeps location.search, so filters and sort stay intact when moving within the People section.

One small improvement opportunity is the sex filter: instead of keeping sex as an empty string for “All”, you could delete the sex param entirely to keep the URL cleaner and more explicit about “no sex filter” being active, but this is a refinement rather than a requirement. Overall, your code is coherent, meets the functional goals, and demonstrates a strong grasp of React Router and search-param handling. Your solution is approved—nice work continuing to refine the behavior around filters, sorting, and navigation.

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

Rate AI review example ↩

solution

afd8b81

mateacademy-ai-mentor suggested changes Jun 19, 2026

View reviewed changes

Ivadriil added 2 commits June 20, 2026 21:33

add task solution

2893899

add task solution

4d69818

Ivadriil requested a review from mateacademy-ai-mentor June 29, 2026 09:53

mateacademy-ai-mentor suggested changes Jun 29, 2026

View reviewed changes

add task solution

4956580

Ivadriil requested a review from mateacademy-ai-mentor June 29, 2026 10:15

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

View reviewed changes

		if (typeof valA === 'number' && typeof valB === 'number') {
		return order !== null ? valB - valA : valA - valB;

Uh oh!

Conversation

Ivadriil commented Jun 19, 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!

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