Solution by Rostyslav452 · Pull Request #1832 · mate-academy/react_people-table-advanced

Rostyslav452 · 2026-06-28T01:42:08Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still a few critical, blocking issues that violate the task requirements and cause tests to fail. First, the century filter uses the search param key century throughout (PeopleFilters.tsx, PeoplePage.tsx, peopleManager.ts), but the specification explicitly requires a centuries param using append/getAll [CHECKLIST ITEM #8, #19, #20]; you need to rename this key consistently and keep using getAll('centuries') in the filtering logic. Second, sorting in PeoplePage.tsx relies on local sortField/sortOrder state that is only initialized once from searchParams and never updated; this breaks the required three‑step click cycle (asc → desc → disabled) driven by the URL params [CHECKLIST ITEM #9–#13, #21, #22], so handleSortField should derive the current sort and order directly from searchParams each time. Third, navigation within the People page does not preserve search params: PersonLink in PeopleFilters.tsx and the top‑level People nav link in App.tsx drop the current query string, which violates the requirement to keep filters/sort when selecting a person or clicking the People link [CHECKLIST ITEM #3, #15]; switch these to use SearchLink or build to using the current location.search. Finally, Cypress reports a failing test where an error message is shown when the API returns an empty list of people; your loading/error logic in PeoplePage.tsx should treat an empty array as a valid "no people" state (no .has-text-danger), and only display the error when the request actually fails. Aside from these issues, your name, sex, and century filtering behaviors and the general routing structure are solid, so once you address these points your solution will be in good 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-06-28T01:43:50Z

+  const [searchParams, setSearchParams] = useSearchParams();
+  const sex = searchParams.get('sex') || '';
+  const query = searchParams.get('query') || '';
+  const currentCenturies = searchParams.getAll('century');


You are using searchParams.getAll('century') and the century key throughout the filter logic, but the description requires a centuries search param and explicitly mentions using append/getAll with that key (checklist items #8, #19, #20). Consider renaming this param to centuries consistently across filters and the filtering utility.

mateacademy-ai-mentor · 2026-06-28T01:43:50Z

+  function handleQueryChange(event: React.ChangeEvent<HTMLInputElement>) {
+    setSearchParams(
+      getSearchWith(searchParams, { query: event.target.value || null }),


The name filter correctly updates query and removes it when the input is empty, which matches the requirements. However, for search param naming consistency, verify that other filters (like centuries) also follow the exact keys specified in the description (centuries rather than century).

mateacademy-ai-mentor · 2026-06-28T01:43:50Z

+                params={{
+                  century: currentCenturies.includes(century)
+                    ? currentCenturies.filter(cent => cent !== century)
+                    : [...currentCenturies, century],


The century filter logic uses century as the search param key and toggles values correctly, but the task description specifies a centuries search param (with append/getAll). To fully comply with checklist items #8, #19, #20, you should switch to using centuries here and in the corresponding filtering logic.

mateacademy-ai-mentor · 2026-06-28T01:43:50Z

+  const sex = searchParams.get('sex') || '';
+  const query = searchParams.get('query') || '';
+  const currentCenturies = searchParams.getAll('century');


The PersonLink navigates to /people/${person.slug} without preserving the current search params (filters/sort). Checklist items #3 and #15 require keeping search params when navigating within the People page, including when selecting a person, so consider using SearchLink or building the to with the current search to retain the query string.

mateacademy-ai-mentor · 2026-06-28T01:43:50Z

+
+export const PersonLink = ({ person }: { person: Person }) => (
+  <Link
+    to={`/people/${person.slug}`}


You are using the search param key 'century' here, but the task specification and getSearchWith comments describe using centuries with append/getAll. To align with checklist items #8, #19, and #20, switch to 'centuries' consistently (here and in filterPeople).

mateacademy-ai-mentor · 2026-06-28T01:43:50Z

+  const [sortField] = useState(searchParams.get('sort'));
+  const [sortOrder] = useState(searchParams.get('order'));


sortField and sortOrder are read once from searchParams into local state and never updated. After the first sort change, these values become stale, so the click-cycle logic (asc → desc → disabled) no longer reflects the current URL params. Consider deriving current sort field and order directly from searchParams inside handleSortField instead of storing them in separate state.

mateacademy-ai-mentor · 2026-06-28T01:43:50Z

+  function handleSortField(field: Field) {
+    let newOrder = 'asc';
+
+    if (field === sortField) {
+      if (sortOrder === 'asc') {
+        newOrder = 'desc';
+      } else if (sortOrder === 'desc') {
+        newOrder = '';
+      }
+    }


In handleSortField, the logic relies on sortField/sortOrder from local state, but those don’t change when you call setSearchParams. This breaks the required three-step sort cycle (ascending, descending, disabled). Use the latest searchParams.get('sort') and searchParams.get('order') within this function to ensure behavior matches checklist items #9–#12.

mateacademy-ai-mentor · 2026-06-28T01:43:50Z

+    setSearchParams(newParams);
+  }
+
+  let preparedPeople = filterPeople(people, searchParams);


The century filtering logic reads searchParams.getAll('century'), but the requirement calls for a centuries search param using append/getAll. To meet checklist item #20, change this to getAll('centuries') and keep the key consistent with PeopleFilters.

mateacademy-ai-mentor · 2026-06-28T01:43:50Z

+  <Link
+    to={`/people/${person.slug}`}


Person links navigate to /people/${person.slug} without preserving current search params, so filters and sorting are lost when selecting a person. Checklist items #3 and #15 require keeping search params when navigating within the People page; consider using SearchLink here or building the to value from the current location/search so the query string is retained.

mateacademy-ai-mentor · 2026-06-28T01:43:50Z

+  people: Person[],
+  searchParams: URLSearchParams,
+): Person[] {
+  const centuryParam = searchParams.getAll('century') || [];


The century filter reads values via searchParams.getAll('century'), but the task description specifies using a centuries search param with append/getAll (checklist items #8, #19, #20). To match the spec, change this to getAll('centuries') and ensure the UI uses the same key.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still a couple of critical, requirement-breaking issues that affect core functionality. First, the century filter reset logic uses the wrong search param key: both the “All” century button and the “Reset all filters” button are clearing century instead of centuries, while your filtering code reads searchParams.getAll('centuries') [CHECKLIST ITEM #7, #14]. This means clicking “All” or resetting filters does not actually remove century filters, so users cannot return to “all centuries” as required; please change those params to use centuries: null and ensure century filters are fully cleared. Second, PersonLink navigations such as to={/people/${person.slug}} drop the current query string, so filters and sorting stored in URL params are lost when selecting a person [CHECKLIST ITEM #2]. You should update PersonLink to preserve the existing search params (for example, by using your SearchLink helper or constructing a to object that includes the current search). Aside from these issues, your implementation of filtering, sorting, and empty vs error handling is solid, so once you fix these points the solution will meet all the checklist 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-28T12:28:20Z

              data-cy="centuryALL"
              className="button is-success is-outlined"
-              href="#/people"
+              params={{ century: null }}


The century filter’s "All" button is using params={{ century: null }}, but all other century filtering logic uses the centuries key with getAll('centuries'). According to the description (checklist items #7 and #14), you must consistently use centuries and be able to clear this filter. Consider changing this to centuries: null so clicking "All" actually removes all centuries search params.

mateacademy-ai-mentor · 2026-06-28T12:28:20Z

+          params={{
+            sex: null,
+            query: null,
+            century: null,


Similarly, the "Reset all filters" button clears sex and query but uses century: null instead of centuries: null. Since your filtering logic reads from searchParams.getAll('centuries'), this reset currently doesn’t disable the century filter. Update this to clear centuries to fully reset filters as required by the task.

mateacademy-ai-mentor · 2026-06-28T12:28:20Z

+  const sex = searchParams.get('sex') || '';
+  const query = searchParams.get('query') || '';
+  const currentCenturies = searchParams.getAll('centuries');


PersonLink uses a plain Link with to={/people/${person.slug}}, which drops the current search params (filters and sorting) when navigating to a person. Checklist item #2 requires keeping search params when navigating within the People page, including when selecting a person. Consider switching this to use SearchLink or construct to with the current location.search so that the URL query string is preserved.

mateacademy-ai-mentor · 2026-06-28T12:28:20Z

+    () =>
+      Array.from(
+        new Set(people.map(person => Math.ceil(person.born / 100).toString())),
+      ).sort(),


The century "All" button uses params={{ century: null }}, but the task requires the centuries search param, and your filtering logic reads getAll('centuries'). Using century here means the All button will not clear the actual century filter. Change this to centuries: null so it removes all centuries values as specified in checklist items #7 and #14.

mateacademy-ai-mentor · 2026-06-28T12:28:20Z

+
+  return (
+    <main className="section">
+      <h1 className="title">People Page</h1>


Similarly, the Reset all filters button is clearing century, but your filter logic and requirements use centuries. This mismatch prevents full reset of century filters; update this to centuries: null so the reset button correctly removes century-related search params.

mateacademy-ai-mentor · 2026-06-28T12:28:20Z

+
+export const PersonLink = ({ person }: { person: Person }) => (
+  <Link
+    to={`/people/${person.slug}`}


PersonLink currently navigates with to={/people/${person.slug}} which drops the current search params. Checklist item #2 requires keeping search params when selecting a person within the People page. Consider using useSearchParams or your SearchLink component to build a to that preserves the existing query string while navigating to person.slug.

mateacademy-ai-mentor · 2026-06-28T12:28:20Z

+    case 'sex':
+      if (orderParam === 'desc') {
+        return people.toSorted((a, b) =>


PersonLink currently navigates with to={/people/${person.slug}}, which does not include the current search string. This causes filters and sorting stored in URL params to be lost when selecting a person, violating checklist item #2 (“Keep search params when navigating within the People page”). Consider using SearchLink here or constructing to with a search based on the current searchParams/location.search so the query string is preserved.

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override so you can continue progressing while you learn, even though there’s one remaining requirements-breaking issue. Overall, you’ve done a strong job wiring filters and sorting to URL search params: name, sex, centuries, and sort/order are all correctly read from and written to searchParams, and the sort click-cycle and navigation behavior (preserving search params when clicking People or a PersonLink) are correctly implemented [CHECKLIST ITEM #2]. The century filter itself is also well done, using centuries with getAll and supporting multiple selections plus an “All” option that clears the century filters as required.

The main issue to fix is the Reset all filters button: in PeopleFilters.tsx and PeoplePage.tsx, it clears sex and query correctly, but uses century: null instead of centuries: null, so century filters are not actually reset in the URL [CHECKLIST ITEM #7, #14]. This breaks the expectation that “Reset all filters” returns the table to the “all centuries” state; to correct it, update the reset params object to use the centuries key consistently (hint: match the key you use in searchParams.getAll('centuries')). As minor polish, you could remove unreachable break statements after return in sortPeople and consider centralizing your fetch logic for people into a shared helper, but these don’t affect correctness.

Your handling of case-insensitive name/motherName/fatherName filtering, multi-century selection, and the conditional display of the filters sidebar when people are loaded shows good understanding of React and URL-based state. Once you adjust the centuries key in the reset-all button, your implementation will fully match the advanced table requirements. Keep up the good work—this is a solid solution with just a small detail left to refine.

✨ 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-06-28T13:03:23Z

+  function handleQueryChange(event: React.ChangeEvent<HTMLInputElement>) {
+    setSearchParams(
+      getSearchWith(searchParams, { query: event.target.value || null }),
+    );


The NameFilter logic correctly updates the query search param and removes it when the input is empty. This matches the task requirement, but note that the filtering behavior itself is implemented in filterPeople, not here, so both pieces together satisfy the NameFilter spec.

mateacademy-ai-mentor · 2026-06-28T13:03:23Z

+  const [searchParams, setSearchParams] = useSearchParams();
+  const sex = searchParams.get('sex') || '';
+  const query = searchParams.get('query') || '';
+  const currentCenturies = searchParams.getAll('centuries');


currentCenturies correctly reads all centuries values via getAll, which matches the requirement to work with multi-value centuries params.

mateacademy-ai-mentor · 2026-06-28T13:03:23Z

+            {centuries.map(century => (
+              <SearchLink
+                key={century}
+                data-cy="century"
+                className={classNames('button', 'mr-1', {
+                  'is-info': currentCenturies.includes(century),
+                })}
+                params={{
+                  centuries: currentCenturies.includes(century)
+                    ? currentCenturies.filter(cent => cent !== century)
+                    : [...currentCenturies, century],


The per-century toggle correctly builds a new centuries array and passes it into getSearchWith, which will translate to multiple centuries params. This satisfies the multi-century filter requirement.

mateacademy-ai-mentor · 2026-06-28T13:03:23Z

+            <SearchLink
              data-cy="centuryALL"
              className="button is-success is-outlined"
-              href="#/people"
+              params={{ centuries: null }}


Good: the "All" century button now properly clears the centuries search params by passing centuries: null, which resets the century filter as required.

mateacademy-ai-mentor · 2026-06-28T13:03:23Z

+        <SearchLink
+          className="button is-link is-outlined is-fullwidth"
+          params={{
+            sex: null,
+            query: null,
+            century: null,
+          }}


Here the reset-all button clears sex and query, but uses century: null instead of centuries: null. Since your filter logic reads searchParams.getAll('centuries'), this means century filters are not reset by this button, violating the requirement that reset must clear the centuries filters [CHECKLIST ITEM #7, #14]. Consider changing this key to centuries so all century filters are removed.

mateacademy-ai-mentor · 2026-06-28T13:03:23Z

+    setSearchParams(
+      getSearchWith(searchParams, { query: event.target.value || null }),
+    );
+  }
+
  return (


Using useLocation and passing search: location.search ensures that when navigating to a person, the current filter/sort search params are preserved, meeting the requirement to keep search params when selecting a person [CHECKLIST ITEM #2].

mateacademy-ai-mentor · 2026-06-28T13:03:23Z

+              <SearchLink
+                key={century}
+                data-cy="century"
+                className={classNames('button', 'mr-1', {
+                  'is-info': currentCenturies.includes(century),
+                })}
+                params={{
+                  centuries: currentCenturies.includes(century)
+                    ? currentCenturies.filter(cent => cent !== century)
+                    : [...currentCenturies, century],
+                }}
+              >
+                {century}
+              </SearchLink>
+            ))}
          </div>

          <div className="level-right ml-4">
-            <a
+            <SearchLink


The sorting handler correctly implements the 3-step cycle (asc → desc → off) and manages the sort and order search params accordingly (removing both when sorting is disabled), which matches the specification.

mateacademy-ai-mentor · 2026-06-28T13:03:23Z

+
+  const showFilters = !isLoading && isLoaded && !isError && people.length > 0;
+  const showLoader = isLoading;
+  const showError = !isLoading && isError;


The reset button still clears century instead of centuries, so existing century filters will not be removed when the user clicks “Reset all filters”. According to the requirements (and checklist items #7 and #14), this button must fully clear the centuries search param; update this object to use centuries: null instead of century: null.

mateacademy-ai-mentor · 2026-06-28T13:03:23Z

+  const centuryParam = searchParams.getAll('centuries') || [];
+  const sexParam = searchParams.get('sex') || '';
+  const queryParam = searchParams.get('query')?.toLowerCase() || '';


Using getAll('centuries') here is correct and aligns with the requirement that the century filter work with multi-value centuries search params; this function will properly respect whatever the UI writes under that key.

Solution

afb709b

mateacademy-ai-mentor suggested changes Jun 28, 2026

View reviewed changes

Rostyslav452 added 2 commits June 28, 2026 04:54

Solution

ebfc01d

Solution

41357d9

Rostyslav452 requested a review from mateacademy-ai-mentor June 28, 2026 12:27

mateacademy-ai-mentor suggested changes Jun 28, 2026

View reviewed changes

Rostyslav452 added 4 commits June 28, 2026 15:40

Solution

1f29585

Solution

531310e

Solution

5fa7fa2

Solution

3ca4812

Rostyslav452 requested a review from mateacademy-ai-mentor June 28, 2026 13:02

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

View reviewed changes

		const [sortField] = useState(searchParams.get('sort'));
		const [sortOrder] = useState(searchParams.get('order'));

Uh oh!

Conversation

Rostyslav452 commented Jun 28, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants