develop#1175
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 one critical blocker that violates an explicit checklist requirement: when the user enters only spaces, the filter runs with whitespace instead of returning all people as required by checklist item #1. The appliedQuery.trim() === '' check must be added to return all people before filtering. Additionally, the selection clearing in handleChange is too aggressive—onSelected(null) clears the selection on every keystroke, which prevents users from editing a selected person's name (e.g., adding a space). The selection should only clear when the input meaningfully differs from the selected person's name, not on every character input.
✨ 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 visiblePeople = useMemo(() => { | ||
| return people.filter(person => | ||
| person.name.toLowerCase().includes(appliedQuery.toLowerCase()), | ||
| ); |
There was a problem hiding this comment.
This violates checklist item #1: filter is called when user enters spaces only. The filter runs with whitespace (appliedQuery.toLowerCase().includes(' ')) instead of skipping the filter when input is only whitespace. Consider checking if appliedQuery.trim() is empty and returning all people in that case.
| const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| setQuery(event.target.value); | ||
| setIsOpen(true); | ||
| onSelected(null); |
There was a problem hiding this comment.
The selection clearing is too aggressive. Currently onSelected(null) is called on every keystroke, which clears the selection even when the user just adds a space to the selected person's name. Per the requirements, selection should only be cleared when the input value changes meaningfully (e.g., user clears or modifies the name significantly). Consider only clearing when !query.startsWith(selectedPerson.name) to allow minor edits.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is complete and all requirements are satisfied. The Autocomplete component correctly handles the spaces-only input case by returning all people when appliedQuery.trim() === '', and the selection clearing logic only triggers when the input meaningfully differs from the selected person's name, allowing users to edit names. All checklist items pass, including proper React usage without direct DOM manipulation, stable keys using person.slug, and correct event handler naming conventions. The h1 displays the selected person's birth-death dates appropriately, and all data-qa attributes are present for testing.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK