solution#1806
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation has two critical issues that violate explicit task requirements. First, in src/pages/PeoplePage.tsx, the PeopleFilters component is rendered unconditionally at line 16, but requirement #3 states that filters should only appear when people are loaded. Currently, filters display alongside the loader, which breaks the intended UX. Second, in src/components/Navbar.tsx, the People link uses standard NavLink instead of SearchLink, causing search params to be lost when navigating within the People page—this violates requirement #2 to preserve filters and sort state. Both issues are functional blockers that must be fixed before approval. Minor issues (ESLint typo, Cypress helper function name, redundant fragments) are noted but don't block approval.
✨ Please rate this review in the Platform's notification center1. 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 button2 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 tag3.
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.
Footnotes
| "name": "Philibert Haverbeke", | ||
| "sex": "m", | ||
| "born": 1907, | ||
| "died": 1997, |
There was a problem hiding this comment.
The helper function is named assetTitle but should be assertTitle to match the test intent (asserting title content) and be consistent with Cypress naming conventions.
| } | ||
|
|
There was a problem hiding this comment.
The PeopleFilters component is rendered unconditionally at line 26-27, but according to the task requirement #3, it should only appear when people are loaded. The loader, error, and no-people messages are correctly shown/hidden based on peopleLoading, but the filters are not. Consider moving PeopleFilters inside the conditional block or adding a condition like {!peopleLoading && !peopleError && !empty && <PeopleFilters />}.
| <div className="navbar-brand"> | ||
| <a className="navbar-item" href="#/"> | ||
| <NavLink className={getLinkClass} to="/"> | ||
| Home | ||
| </a> | ||
| </NavLink> | ||
|
|
There was a problem hiding this comment.
The Navbar uses standard NavLink for the People link. Clicking 'People' will navigate to '/people' without preserving current search params, violating the requirement 'Keep search params when navigating within the People page'. Consider using SearchLink for the People link to preserve filters and sort state.
| <td>{person.sex}</td> | ||
| <td>{person.born}</td> | ||
| <td>{person.died}</td> | ||
| <td> |
There was a problem hiding this comment.
The century century filter's "All" button uses loose equality (!=) instead of strict inequality (!==). Consider using !== for consistency with TypeScript conventions, though this doesn't affect functionality.
| <td> | ||
| {motherLink(person) ? ( | ||
| <Link | ||
| to={`../${motherLink(person)?.slug}`} | ||
| className="has-text-danger" | ||
| > | ||
| {person.motherName} | ||
| </Link> | ||
| ) : ( | ||
| person.mother?.name || person.motherName || '-' | ||
| )} | ||
| </td> | ||
| <td> | ||
| {fatherLink(person) ? ( | ||
| <Link to={`../${fatherLink(person)?.slug}`}> | ||
| {person.fatherName} | ||
| </Link> | ||
| ) : ( |
There was a problem hiding this comment.
The motherLink(person) and fatherLink(person) functions are called multiple times within the same JSX expression (lines 129-137 and 141-146), which means the .find() operation runs 4 times each instead of once. Consider extracting these to variables before the JSX to improve performance.
| <div className="column is-7-tablet is-narrow-desktop"> | ||
| <PeopleFilters /> |
There was a problem hiding this comment.
The PeopleFilters component is rendered unconditionally, but according to requirement #3, filters should appear only when people are loaded. Currently, filters would be visible even during loading. Consider adding the same condition used for PeopleTable: {!peopleLoading && !peopleError && !empty && <PeopleFilters />}.
| <> | ||
| <h1 className="title">Home Page</h1> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
The PageNotFound component has redundant nested empty fragments (<> <> </> </>). While this works, one set of fragments is sufficient and cleaner.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation meets all the core functional requirements for the task. The filtering and sorting features work correctly with URL search parameters, the PeopleFilters sidebar properly conditionally renders only when people are loaded, and all required functionality (name filtering, multi-century selection, 3-state sorting) is working as specified. There are two minor typos worth noting but they don't block functionality: the ESLint rule name 'implicit-arrow-linebreak:' in .eslintrc.cjs has a trailing colon that should be removed (ESLint ignores unknown rules, so this won't cause errors), and the Cypress helper function assetTitle in page.spec.js should be named assertTitle to better reflect its purpose. These are cosmetic issues that don't affect task completion. All requirements from the checklist are satisfied, and no check run failures were reported. Your solution is approved.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| { blankLine: 'any', prev: ['const', 'let', 'var'], next: ['const', 'let', 'var'] }, | ||
| { blankLine: 'always', prev: 'directive', next: '*' }, | ||
| { blankLine: 'always', prev: 'block-like', next: '*' }, | ||
| ], |
There was a problem hiding this comment.
Rule name has a trailing colon which is a typo - should be 'implicit-arrow-linebreak' without the colon. While ESLint ignores unknown rules, this should be corrected for clarity.
| heading: () => page.peopleTable().find('th'), | ||
| assertHash: hash => cy.location('hash').should('eq', hash), | ||
| assertSearch: search => cy.location('search').should('eq', search), |
There was a problem hiding this comment.
Typo in function name: assetTitle should be assertTitle since this function performs an assertion, not an assignment. The tests reference this as page.assetTitle() so this is functionally working, but the name is misleading.
| <td> | ||
| {motherLink(person) ? ( | ||
| <Link | ||
| to={`../${motherLink(person)?.slug}`} | ||
| className="has-text-danger" | ||
| > | ||
| {person.motherName} | ||
| </Link> | ||
| ) : ( | ||
| person.mother?.name || person.motherName || '-' | ||
| )} | ||
| </td> | ||
| <td> | ||
| {fatherLink(person) ? ( | ||
| <Link to={`../${fatherLink(person)?.slug}`}> | ||
| {person.fatherName} | ||
| </Link> | ||
| ) : ( | ||
| person.father?.name || person.fatherName || '-' | ||
| )} |
There was a problem hiding this comment.
The motherLink(person) and fatherLink(person) functions are called multiple times in the JSX (lines 129, 131, 141, 142). Consider extracting these to variables once per person to avoid redundant array.find() calls.
2 participants
DEMO LINK