Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the core features of the Autocomplete component! The debouncing and state management in the App component are well done. However, a couple of changes are needed to meet all the requirements.
First, the filter is triggered even when the input contains only spaces, which goes against a specific requirement [CHECKLIST ITEM #1]. You can fix this by using .trim() on the query within your useEffect hook before deciding whether to run the filter.
Second, after selecting a person, if you click away and then re-focus the input, the suggestion list is empty. This is because the filteredPeople state is cleared on selection. Removing the line that sets setFilteredPeople([]) in your selection handler will resolve this, allowing the useEffect hook to correctly repopulate the suggestions.
Please address these two points. I'm looking forward to seeing the updated version!
✨ 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 handler = setTimeout(() => { | ||
| const normalizedQuery = query.toLowerCase(); | ||
|
|
||
| setFilteredPeople( | ||
| query | ||
| ? people.filter(person => | ||
| person.name.toLowerCase().includes(normalizedQuery), | ||
| ) | ||
| : people, | ||
| ); | ||
| lastRefQuery.current = query; | ||
| }, delay); |
There was a problem hiding this comment.
This effect doesn't handle the case where the query consists only of whitespace. This violates checklist item #1: 'make sure that filter won't be called if user enred spaces only'. You should trim() the query before using it to decide whether to filter.
src/components/Autocomplete.tsx
Outdated
| setQuery(person.name); | ||
| setIsOpen(false); | ||
| lastRefQuery.current = person.name; | ||
| setFilteredPeople([]); |
There was a problem hiding this comment.
Clearing filteredPeople here leads to an empty suggestion list if the user re-focuses the input after making a selection. Consider removing this line. The useEffect hook will correctly repopulate the suggestions when query is updated, and since the dropdown is closed, the user won't see the intermediate state.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this update! You've successfully addressed all the feedback from the previous review, and the component now functions perfectly according to all requirements. I am approving your submission. As a minor suggestion, you can simplify the logic inside the useEffect in src/components/Autocomplete.tsx. Since you already handle the case where query.trim() === '' at the top of the hook, the ternary operator used for filtering is redundant. This is just a small refinement, and your current implementation is solid. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| query | ||
| ? people.filter(person => | ||
| person.name.toLowerCase().includes(normalizedQuery), | ||
| ) | ||
| : people, |
There was a problem hiding this comment.
Since you are already handling the case where query.trim() === '' earlier in the useEffect hook, the query variable will always be a non-empty string at this point. This makes the ternary operator redundant.
You can simplify this logic by removing the check and the else branch, leaving only the filtering logic.
2 participants
DEMO LINK