Solution by azesmmisha · Pull Request #1834 · mate-academy/react_people-table-advanced

azesmmisha · 2026-07-01T14:31:10Z

https://azesmmisha.github.io/react_people-table-advanced/

mateacademy-ai-mentor

Review Summary

I am requesting changes because a few core checklist requirements are still not met and will likely cause test failures. The biggest blocker is the century search param: you consistently use century, but the task explicitly requires centuries (plural) with append/getAll in PeopleFilters, PeopleTable, PeoplePage, and PeopleContext [CHECKLIST ITEM #20–#22]; this needs to be renamed and aligned everywhere.

A second critical issue is that navigation within the People page does not preserve search params: the Navbar link to /people and the person links (PersonLink, table row links, index.tsx) all drop existing filters/sort from the URL instead of carrying them forward via SearchLink or useSearchParams [CHECKLIST ITEM #4, #16]. There is also a logic bug where the "There are no people matching the current search criteria" message is shown when visiblePeople.length > 0 instead of when there are no visible people, so users see the message at the wrong time. To fix these, 1) switch all century-related keys and getAll calls to centuries, 2) refactor navigation on the People page to reuse the shared SearchLink/getSearchWith helpers or manually include current searchParams in the to prop, and 3) adjust the condition so the empty-results message appears only when filters lead to zero matches.

Once these issues are addressed, your implementation will align well with the required behavior; the rest of your filtering and sorting logic is solid and very close to complete.

✨ 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-07-01T14:32:55Z

+          <NavLink className={navClass} to="/">
+            Home
+          </NavLink>
+          <NavLink className={navClass} to="/people">


The task requires keeping URL search params when navigating within the People page (checklist items #3 and #16). Using a plain NavLink to /people will drop any existing search params; consider switching to the provided SearchLink helper or otherwise preserving searchParams when navigating to the People route.

mateacademy-ai-mentor · 2026-07-01T14:32:55Z

+    <nav
+      data-cy="nav"
+      className="navbar is-fixed-top has-shadow"
+      role="navigation"
+      aria-label="main navigation"
+    >
+      <div className="container">
+        <div className="navbar-brand">
+          <NavLink className={navClass} to="/">
+            Home
+          </NavLink>
+          <NavLink className={navClass} to="/people">
+            People
+          </NavLink>
+        </div>


You already have a shared getSearchWith/SearchParams helper in src/utils/searchHelper.ts per the task description. Duplicating a local version here instead of importing and using the common helper can lead to inconsistency and diverges from checklist item #17, which expects reuse of the provided code.

mateacademy-ai-mentor · 2026-07-01T14:32:55Z

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


According to checklist items #20–#22, the century filter should use a centuries search param and rely on append/getAll. Here you use century as the key, which will break alignment with the specified API and tests expecting centuries. Consider renaming the key to centuries and reusing the shared search helper instead of this local version.

mateacademy-ai-mentor · 2026-07-01T14:32:55Z

+function getSearchWith(params: Params, search?: string | URLSearchParams) {
+  const newParams = new URLSearchParams(search);
+
+  for (const [key, value] of Object.entries(params)) {
+    if (value === null) {
+      newParams.delete(key);
+    } else if (Array.isArray(value)) {
+      newParams.delete(key);
+      value.forEach(item => newParams.append(key, item.toString()));
+    } else {
+      newParams.set(key, value.toString());
+    }
+  }
+
+  return newParams.toString();
+}


The helper function getSearchWith here duplicates the logic already provided in src/utils/searchHelper.ts and uses a different Params type (string | number instead of string | string[] | null). This conflicts with the guideline to reuse the existing helper and can cause inconsistencies. Refactor to import and use the shared getSearchWith (and possibly SearchLink) instead of reimplementing it.

mateacademy-ai-mentor · 2026-07-01T14:32:55Z

+            {[16, 17, 18, 19, 20].map(century => {
+              return (
+                <Link
+                  key={century}
+                  data-cy="century"
+                  className={`button mr-1 ${centuries.includes(century.toString()) ? 'is-info' : ''}`}
+                  to={{
+                    search: getSearchWith(
+                      { century: toggledCenturies(century) },
+                      searchParams,
+                    ),
+                  }}
+                >
+                  {century}
+                </Link>
+              );
+            })}
+          </div>
+
+          <div className="level-right ml-4">
+            <Link
+              data-cy="centuryALL"
+              className="button is-success is-outlined"
+              to={{
+                search: getSearchWith({ century: null }, searchParams),
+              }}
+            >
+              All
+            </Link>
+          </div>
+        </div>
+      </div>
+
+      <div className="panel-block">
+        <Link
+          className="button is-link is-outlined is-fullwidth"
+          to={{
+            search: getSearchWith(
+              { sex: null, century: null, query: null },


In the century toggling logic and links you again use century as the search key, but the task requires centuries (plural) along with append/getAll. Update these uses to centuries to comply with the requirement and keep your filtering logic in PeoplePage consistent.

mateacademy-ai-mentor · 2026-07-01T14:32:55Z

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


Checklist items #4 and #16 require keeping search params when navigating within the People page, including when selecting a person. This plain Link to /people/${person.slug} will drop any existing filters/sort params. Consider using useSearchParams and passing them into to (or using the SearchLink helper) so the current search params are preserved.

mateacademy-ai-mentor · 2026-07-01T14:32:55Z

+  <HashRouter>
    <App />


Checklist items #4 and #16 require keeping search params when selecting a person on the People page. This plain Link to /people/${person.slug} discards existing filters and sort params from the URL. Consider using useSearchParams with SearchLink (and getSearchWith) so the to value includes the current search string plus the slug.

mateacademy-ai-mentor · 2026-07-01T14:32:55Z

+
+  const sex = searchParams.get('sex') || '';
+  const query = searchParams.get('query') || '';
+  const centuries = searchParams.getAll('century') || [];


The century filter currently reads searchParams.getAll('century'), but the task description and checklist specify using a centuries search param with append/getAll. To match the required API and your filter component, you should switch this key to centuries and keep it consistent across the app.

mateacademy-ai-mentor · 2026-07-01T14:32:55Z

+              {showTable && (
+                <p>There are no people matching the current search criteria</p>


showTable is true when visiblePeople.length > 0, yet you use it to show the "There are no people matching the current search criteria" message. This results in the message appearing when there are matches and not appearing when there are none. The condition should be based on visiblePeople.length === 0 (and no loading/error) to correctly indicate that filters produced no results.

mateacademy-ai-mentor · 2026-07-01T14:32:55Z

+  loading: boolean;
+  error: boolean;
+};
+


Century filtering here uses searchParams.getAll('century'), but the task requires a centuries search param key and the use of append/getAll on that key (checklist items #20–#22). To stay consistent with PeopleFilters and the spec, change this to getAll('centuries') and update related code.

mateacademy-ai-mentor

Review Summary

I am requesting changes because one critical requirement is still not met: navigation within the People page does not preserve the current search params (filters and sorting), which is explicitly required and will break expected behavior. Specifically, the navbar People link (e.g. in src/App.tsx and src/components/NavBar/Navbar.tsx) and the PersonLink component (e.g. src/components/PersonLink/PersonLink.tsx, also used in PeopleTable and index.tsx) use plain path strings like "/people" and "/people/${person.slug}", so query, sex, centuries, sort, and order are lost when clicking them [CHECKLIST ITEM #4, #16]. You’ve correctly implemented filtering, century handling with centuries + getAll, the three-click sorting cycle via sort and order=desc, and the empty-results message logic, so the core data and UI behavior are solid.

The main fixes needed are:

Update the navbar People link to keep the current search string by using SearchLink or useSearchParams plus a to object like { pathname: '/people', search: currentSearch }.
Update PersonLink (and any direct Link/NavLink to /people/:slug) to preserve existing search params when changing only the pathname, again via SearchLink or useSearchParams + getSearchWith.

Once these links carry forward the active filters and sort state, your solution will fully align with the task requirements. You’ve done a good job on the more complex filtering and sorting logic—focus now on this navigation detail to complete the implementation.

✨ 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-07-01T15:14:35Z

+            <Route
+              path="/people/:slug?"


The route setup is fine, but when PeoplePage is rendered via /people/:slug?, internal navigation to this route (from Navbar or person links) must preserve existing search params as per the description. Right now, the Navbar and PersonLink use plain paths without including current searchParams, so filters/sort are lost when navigating.

mateacademy-ai-mentor · 2026-07-01T15:14:35Z

+            <Route path="/home" element={<Navigate to="/" replace />} />
+            <Route path="/" element={<HomePage />} />
+            <Route
+              path="/people/:slug?"


This NavLink to /people drops any existing search params (filters/sort) when users click the "People" link. The requirements (checklist #4, #16) say search params must be kept when navigating within the People page, so consider using SearchLink or useSearchParams here to include the current query/sex/centuries/sort/order in the to prop.

mateacademy-ai-mentor · 2026-07-01T15:14:35Z

      <Navbar />

-      <div className="section">
+      <main className="section">
        <div className="container">
-          <h1 className="title">Home Page</h1>
-          <h1 className="title">Page not found</h1>
-          <PeoplePage />
+          <Routes>
+            <Route path="/home" element={<Navigate to="/" replace />} />
+            <Route path="/" element={<HomePage />} />


This link navigates to /people/${person.slug} but does not include the current search params from the URL, so filters and sorting are lost when selecting a person. The description requires that search params be kept when selecting a person within the People page; consider using SearchLink or useSearchParams to construct a to object that preserves the existing search string while changing only the pathname.

mateacademy-ai-mentor · 2026-07-01T15:14:35Z

+          <NavLink className={navClass} to="/">
+            Home
+          </NavLink>
+          <NavLink className={navClass} to="/people">


The /people NavLink navigates without including the current search params, so clicking “People” will clear active filters and sort. Checklist items #4 and #16 require preserving search params when navigating within the People page; consider using SearchLink or useSearchParams + getSearchWith so the link carries the existing search instead of just "/people".

mateacademy-ai-mentor · 2026-07-01T15:14:35Z

+    >
+      <div className="container">
+        <div className="navbar-brand">
+          <NavLink className={navClass} to="/">


PersonLink builds a plain path string (to={/people/${person.slug}}), which drops any existing search params for filters and sorting. According to the requirements, navigation within the People page (person links, table row links) must keep current search params; you can fix this by replacing Link with SearchLink and passing a params object that doesn’t change filters, or by deriving current searchParams and including them in the to prop.

mateacademy-ai-mentor · 2026-07-01T15:14:35Z

+      newParams.delete(key);
+      value.forEach(item => newParams.append(key, item.toString()));
+    } else {
+      newParams.set(key, value.toString());


The Navbar link to /people does not preserve current search params. According to the task, navigation within the People page must keep filters and sort, so clicking "People" should retain the existing searchParams instead of starting with a clean URL. Consider using SearchLink or useSearchParams here to include the current search string.

mateacademy-ai-mentor · 2026-07-01T15:14:35Z

+    if (value === null) {
+      newParams.delete(key);
+    } else if (Array.isArray(value)) {
+      newParams.delete(key);


PersonLink navigates to /people/${person.slug} without preserving current search params. Checklist items require that selecting a person keeps existing filters and sort in the URL; you should build the to value using the current searchParams (e.g. via useSearchParams and getSearchWith) so the query, sex, centuries, and sort parameters remain when opening a person.

mateacademy-ai-mentor · 2026-07-01T15:14:35Z

+    if (field === 'born' || field === 'died') {
+      return (a[field] - b[field]) * direction;


The /people/:slug? route correctly wraps PeoplePage in PeopleProvider, so data loading and filtering happen within the same context, which is required for the task.

mateacademy-ai-mentor · 2026-07-01T15:14:35Z

+    const direction = order === 'desc' ? -1 : 1;
+
+    if (field === 'born' || field === 'died') {
+      return (a[field] - b[field]) * direction;


This NavLink to /people drops any existing search params (filters and sort). The requirements state that search params must be kept when navigating within the People page (clicking the People link), so you need to preserve the current search here, e.g. by using SearchLink or by composing to={{ pathname: '/people', search: currentSearch }} based on useSearchParams.

mateacademy-ai-mentor · 2026-07-01T15:14:36Z

+  const [searchParams, setSearchParams] = useSearchParams();
+
+  const sort = searchParams.get('sort') || '';


You correctly read sex, query, and centuries from search params, and centuries uses the required plural key with getAll, matching the checklist.

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

+      params.delete('order');
+    } else if (!order) {


The handleQueryChange correctly updates the query search param and removes it when the input is empty (value || null), which matches the NameFilter behavior requirement.

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

+      params.delete('sort');
+      params.delete('order');
+    }
+


Century toggling builds a new array and relies on SearchLink/getSearchWith to append multiple centuries values. This satisfies the requirement to store multiple centuries in centuries using append and read them via getAll.

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

+  const handleSort = (field: SortField) => {
+    const params = new URLSearchParams(searchParams);
+
+    if (sort !== field) {
+      params.set('sort', field);
+      params.delete('order');
+    } else if (!order) {
+      params.set('order', 'desc');
+    } else {
+      params.delete('sort');
+      params.delete('order');
+    }
+
+    setSearchParams(params);


Sorting logic correctly implements the required 3-step cycle using sort and order=desc: first click sets ascending (no order), second click adds order=desc, third click removes both sort and order. This matches the sorting URL state requirements.

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

+              <td>
+                <PersonLink person={person} />


The sorted table correctly renders PersonLink for person names and parents, but PersonLink currently navigates to /people/:slug without preserving the current search params. The requirements say search params must be kept when selecting a person, so you should adjust PersonLink to maintain existing filters/sort (e.g., using SearchLink and getSearchWith with current searchParams).

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

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


PersonLink builds to={/people/${person.slug}}, which drops any existing search params such as query, sex, centuries, sort, and order. The requirements state that search params must be kept when selecting a person within the People page, so this component should preserve the current search (for example, by using SearchLink and passing appropriate params, or by using useSearchParams to build a to={{ pathname: /people/${person.slug}, search }} object).

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

+  <HashRouter>
    <App />


This Link navigates to /people/${person.slug} with no search specified, so any active filters/sorting in the URL are lost when clicking on a person. The task requires keeping search params when selecting a person, so instead of a bare path string, you should preserve the existing search params in the to value.

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

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


Filtering by sex, query, and centuries is implemented correctly here. You use searchParams.getAll('centuries') with the plural key and then map to numbers, which aligns with the requirement to use centuries plus append/getAll semantics in the filters.

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

+  if (query) {
+    filtered = filtered.filter(person =>
+      [person.name, person.motherName, person.fatherName]
+        .map(name => name?.toLowerCase())
+        .some(name => name?.includes(query.toLowerCase())),
+    );


The query filter correctly matches against name, motherName, and fatherName case-insensitively, which fulfills the NameFilter behavior requirement from the description.

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

+  const showNoCriteriaMatch =
+    !loading && !error && people.length > 0 && visiblePeople.length === 0;


The showNoCriteriaMatch flag correctly ensures the "There are no people matching the current search criteria" message is shown only when there are people on the server but visiblePeople.length === 0, fixing the logic bug from the previous review.

mateacademy-ai-mentor · 2026-07-01T15:14:40Z

+          <div className="column is-7-tablet is-narrow-desktop">
+            {!loading && !error && <PeopleFilters />}


Rendering <PeopleFilters /> only when !loading && !error ensures the sidebar appears only when people are loaded, which matches the task requirement.

mateacademy-ai-mentor · 2026-07-01T15:14:44Z

+    filtered = filtered.filter(person =>
+      [person.name, person.motherName, person.fatherName]
+        .map(name => name?.toLowerCase())
+        .some(name => name?.includes(query.toLowerCase())),
+    );
+  }
+
+  if (centuries.length) {
+    const centuryNumbers = centuries.map(Number);
+
+    filtered = filtered.filter(person =>
+      centuryNumbers.includes(Math.ceil(person.born / 100)),
+    );
+  }
+
+  return filtered;
+};
+
+export const PeoplePage = () => {
+  const { people, loading, error } = usePeople();


The context setup and data loading logic are sound: PeopleProvider loads people via getPeople, manages loading/error, and memoizes the context value. This matches the expected store behavior for the task.

mateacademy-ai-mentor · 2026-07-01T15:14:44Z

+type PeopleContextType = {
+  people: Person[];
+  loading: boolean;
+  error: boolean;
+};
+
+export const PeopleContext = React.createContext<PeopleContextType>({
+  people: [],
+  loading: false,
+  error: false,
+});
+
+export const PeopleProvider: React.FC<Props> = ({ children }) => {
+  const [people, setPeople] = useState<Person[]>([]);
+  const [loading, setLoading] = useState(false);
+  const [error, setError] = useState(false);
+
+  useEffect(() => {
+    setLoading(true);
+    getPeople()
+      .then(setPeople)
+      .catch(() => setError(true))
+      .finally(() => setLoading(false));
+  }, []);
+
+  const value = useMemo(
+    () => ({ people, loading, error }),
+    [people, loading, error],


Filter logic correctly applies sex, query, and centuries from searchParams, and the century calculation via Math.ceil(person.born / 100) matches the described behavior for CenturyFilter. No changes needed here.

mateacademy-ai-mentor · 2026-07-01T15:14:44Z

+export const PeopleProvider: React.FC<Props> = ({ children }) => {
+  const [people, setPeople] = useState<Person[]>([]);
+  const [loading, setLoading] = useState(false);
+  const [error, setError] = useState(false);
+
+  useEffect(() => {
+    setLoading(true);
+    getPeople()
+      .then(setPeople)
+      .catch(() => setError(true))
+      .finally(() => setLoading(false));
+  }, []);
+
+  const value = useMemo(
+    () => ({ people, loading, error }),
+    [people, loading, error],
+  );
+
+  return (
+    <PeopleContext.Provider value={value}>{children}</PeopleContext.Provider>
+  );
+};
+
+export function usePeople() {
+  return useContext(PeopleContext);


The context implementation correctly loads people once, exposes people, loading, and error through usePeople, and memoizes the value; this meets the data-loading and state-management needs for the task.

mateacademy-ai-mentor

Review Summary

I am approving your solution — your filtering, sorting, and URL search-params handling on the People page are implemented correctly and align with the task requirements. You did especially well with the three-step sorting cycle, preserving sort and order in the URL, and keeping filters like sex, query, and centuries fully in sync with useSearchParams and the UI. The navigation behavior is also strong: both the navbar People link and PersonLink correctly keep the current search string, so users don’t lose their filters or sort state when moving around the People section.

One area to keep in mind as you continue learning is the process side: make sure you run npm test regularly and keep using Prettier or your formatter so everything stays consistent with the course checklist and style expectations. This approval is meant to support your progress while you’re learning, but your current code already shows solid understanding of React, hooks, and URL-based state management. Keep building on this foundation — your implementation demonstrates careful reading of the requirements and good attention to edge cases like empty queries and multiple centuries.

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

Rate AI review example ↩

Solution

22596d3

mateacademy-ai-mentor suggested changes Jul 1, 2026

View reviewed changes

fix: SearchLink, show content logic, centur(y/ies)

7887bcc

azesmmisha requested a review from mateacademy-ai-mentor July 1, 2026 15:13

mateacademy-ai-mentor suggested changes Jul 1, 2026

View reviewed changes

mateacademy-ai-mentor reviewed Jul 1, 2026

View reviewed changes

fix

fa6e4e0

azesmmisha requested a review from mateacademy-ai-mentor July 1, 2026 16:05

mateacademy-ai-mentor approved these changes Jul 1, 2026

View reviewed changes

		{showTable && (
		<p>There are no people matching the current search criteria</p>

		if (field === 'born' \|\| field === 'died') {
		return (a[field] - b[field]) * direction;

		const [searchParams, setSearchParams] = useSearchParams();

		const sort = searchParams.get('sort') \|\| '';

		const showNoCriteriaMatch =
		!loading && !error && people.length > 0 && visiblePeople.length === 0;

		<div className="column is-7-tablet is-narrow-desktop">
		{!loading && !error && <PeopleFilters />}

Uh oh!

Conversation

azesmmisha commented Jul 1, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!