react_people-table-advanced#1818
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a few critical, behavior‑breaking issues around URL handling and filtering/sorting logic.
Blocking issues that must be fixed:
- In multiple places (e.g.
src/components/PeopleFilters.tsxforhandleQueryChangeandsrc/components/PeopleTable.tsxforhandleSort), you callsetSearchParams(getSearchWith(...)), butsetSearchParamsexpects aURLSearchParamsinstance or an init object, not a raw string. This can cause runtime errors and broken updates tosort,order, and filter params; wrap the result innew URLSearchParams(...)or switch to passing an object instead. - The sex filter is written to the URL in
PeopleFilters, but insrc/components/PeoplePage.tsxyou never usesearchParams.get('sex')to actually filtervisiblePeople, so the UI control doesn’t affect the data. You need to add asex-based filter step (e.g., only include people whosesexmatches thesexparam) before applying sorting. - In
PeoplePage.tsx, you treatsortas a generickeyof Person, which means any arbitrarysortvalue in the URL will be used; per requirements, onlyname,sex,born, anddiedshould be valid. Add a small validation step (e.g., check against an allowed array of fields) and ignore unsupported values so unexpected params don’t change sorting.
Minor but recommended improvements: you can tighten the type of the field parameter in handleSort to 'name' | 'sex' | 'born' | 'died' for clarity, and ensure that when switching columns you clear any stale order so the first click is always treated as ascending with order removed. Once you address the setSearchParams misuse, apply the sex filter, and validate the sort field, your solution should fully match the task requirements—your routing, century and query filters, and general URL‑based state handling are otherwise on the right track.
✨ 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
| const handleQueryChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value; | ||
|
|
||
| setSearchParams(getSearchWith(searchParams, { query: value || null })); |
There was a problem hiding this comment.
setSearchParams expects either a URLSearchParams instance or an init object; here you’re passing the string returned by getSearchWith, which can cause runtime issues. Consider wrapping this into a URLSearchParams (or using setSearchParams with an object) instead of passing the raw string.
| if (sortField) { | ||
| visiblePeople.sort((a, b) => { | ||
| const aValue = a[sortField as keyof Person] || ''; | ||
| const bValue = b[sortField as keyof Person] || ''; | ||
|
|
||
| if (aValue < bValue) { | ||
| return order === 'desc' ? 1 : -1; |
There was a problem hiding this comment.
The current handleSort does not implement the required three-step behavior (asc → desc → disabled). On the first click you should set sort to this field and leave order unset (ascending by default), on the second click toggle order to 'desc', and on the third click clear both sort and order so sorting is disabled (per checklist items #10–13, #20–22).
| if (sortField) { | ||
| visiblePeople.sort((a, b) => { | ||
| const aValue = a[sortField as keyof Person] || ''; | ||
| const bValue = b[sortField as keyof Person] || ''; | ||
|
|
||
| if (aValue < bValue) { | ||
| return order === 'desc' ? 1 : -1; | ||
| } | ||
|
|
||
| if (aValue > bValue) { | ||
| return order === 'desc' ? -1 : 1; | ||
| } | ||
|
|
||
| return 0; | ||
| }); | ||
| } |
There was a problem hiding this comment.
sortField is used as a generic keyof Person, so any sort value in the URL (including unsupported fields) will be used for sorting. The task only requires sorting by name, sex, born, and died; consider validating sortField against this allowed set before applying sorting so unexpected search params don’t affect behavior.
| const handleSort = (field: keyof Person) => { | ||
| // ... твоя логіка сортування залишається без змін ... | ||
| let nextSort: string | null = field; | ||
| let nextOrder: string | null = null; | ||
|
|
||
| if (currentSort === field) { | ||
| if (currentOrder !== 'desc') { | ||
| nextOrder = 'desc'; | ||
| } else { | ||
| nextSort = null; | ||
| nextOrder = null; | ||
| } | ||
| } | ||
|
|
||
| setSearchParams( | ||
| getSearchWith(searchParams, { | ||
| sort: nextSort, | ||
| order: nextOrder, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
handleSort does not fully enforce the 3-step sort cycle and also passes a string to setSearchParams. First, make sure that when switching to a new column you clear any stale order from previous sorts and explicitly represent ascending state (e.g., sort=<field> and order removed). Second, setSearchParams expects a URLSearchParams or an init object, not the raw string from getSearchWith; consider wrapping that string or using an object form.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because all critical functional requirements for this task are met, including filtering, multi‑century selection, URL‑backed state, and the full sorting click cycle with correct sort/order params. The People page correctly keeps filters and sort in the URL, preserves them when navigating via the Navbar and person links, and only shows the filters sidebar when data is successfully loaded and non‑empty. Sorting is limited to the allowed fields (name, sex, born, died), the first click is always ascending with no order, the second adds order=desc, and the third clears both sort and order, matching the checklist behavior.
Some type‑safety and validation details (like making sort validation stricter in PeoplePage or treating order as a more explicit union type) could be refined, but these are non‑blocking improvements rather than functional bugs. From the perspective of the required behavior and URL semantics, your implementation behaves correctly and consistently across components. Nicely done bringing everything into alignment with the spec and prior review feedback.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| setSearchParams( | ||
| new URLSearchParams( | ||
| getSearchWith(searchParams, { query: value || null }), | ||
| ), | ||
| ); | ||
| }; | ||
|
|
||
| // Логіка для додавання/видалення століть | ||
| const getCenturySearch = (century: string) => { | ||
| let newCenturies = [...centuries]; | ||
|
|
||
| if (newCenturies.includes(century)) { | ||
| newCenturies = newCenturies.filter(c => c !== century); | ||
| } else { | ||
| newCenturies.push(century); |
There was a problem hiding this comment.
The checklist requires: (1) first click = ascending with no order param, (2) second click = descending with order=desc, and (3) third click = sorting disabled with no sort and no order. Your handleSort correctly toggles sort and sets order='desc' on the second click, and clears both on the third click, so the URL behavior from the table side matches the requirements.
| } else { | ||
| newCenturies.push(century); |
There was a problem hiding this comment.
isSortValid checks only that sortField is among name/sex/born/died, but it doesn’t validate order. Later you treat order as either 'desc' or “ascending” (any other value), so a stale or manually entered order=desc combined with a new sort value would make the first click appear descending, which violates the requirement that first click is always ascending. Consider validating order (only accept 'desc'), and treating anything else as “no order” so the first click is always ascending.
2 participants
DEMO LINK