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 functionality of the Autocomplete component! However, a couple of changes are needed. The most critical issue is that the filter is triggered even when the input contains only spaces, which violates a specific requirement. Please make sure to trim the input value before deciding whether to filter the suggestions. Additionally, for a better user experience, the suggestions dropdown should close when the user clicks outside of it (on blur). I also noticed a minor typo in a state setter name (setAppleidQuery) and some unused variables that could be cleaned up. You're very close to a complete solution!
✨ 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
| people: Person[]; | ||
| onSelected: (person: Person | null) => void; | ||
| delay?: number; | ||
| } |
There was a problem hiding this comment.
These variables (name, born, died) are declared but never used. It's a good practice to remove unused code to keep components clean.
src/App.tsx
Outdated
| import { Person } from './types/Person'; | ||
|
|
||
| export const App: React.FC = () => { | ||
| const { name, born, died } = peopleFromServer[0]; |
There was a problem hiding this comment.
These variables name, born, and died are destructured but never used within the component. This line can be removed.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback! The component is very close to complete. However, changes are needed because a critical bug prevents the main functionality from working: clicking a suggestion does not select it. This happens in src/Autocomplete.tsx because the input's onBlur event fires before the suggestion's onClick event, causing the dropdown to close before the click can be processed. A common way to fix this is to add a small delay to the onBlur handler using setTimeout.
Additionally, to improve performance, the filtering logic runs on every render. Please wrap this logic in a useMemo hook to ensure it only runs when the debounced query changes. Once the selection functionality is fixed, this will be ready for 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
src/Autocomplete.tsx
Outdated
| const filteredPeople = people.filter(person => | ||
| person.name.toLowerCase().includes(appliedQuery.trim().toLowerCase()), | ||
| ); |
There was a problem hiding this comment.
This filtering logic is re-executed on every render, which occurs on every keystroke. This is inefficient and goes against the requirement to "don't run filtering again if the text has not changed".
To fix this, you can wrap this calculation in a useMemo hook, with appliedQuery and people as dependencies. This will ensure filtering only happens when the debounced query changes.
src/Autocomplete.tsx
Outdated
| onSelected(null); | ||
| }} | ||
| onFocus={() => setIsOpen(true)} | ||
| onBlur={() => setIsOpen(false)} |
There was a problem hiding this comment.
There's a potential issue here. When a user clicks on a suggestion, the onBlur event on the input will likely fire first. This immediately closes the dropdown by setting isOpen to false, which may remove the suggestion item from the DOM before its onClick event has a chance to fire.
A common way to solve this is to add a small delay to closing the dropdown in the onBlur handler, for example using setTimeout.
| <Autocomplete | ||
| people={peopleFromServer} | ||
| onSelected={setSelectedPerson} | ||
| delay={500} | ||
| /> |
There was a problem hiding this comment.
While the props passed to Autocomplete are correct, there's a bug within the component that prevents selection. When the input field's onBlur event is triggered (for example, by clicking a suggestion), the suggestion list closes before the onClick event on an item can be processed.
A common solution is to use the onMouseDown event on the suggestion items inside Autocomplete.tsx instead of onClick, because mousedown fires before blur.
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 version! You've successfully resolved the tricky onBlur/onClick race condition and made a great performance improvement by memoizing the filtering logic.
I am approving this solution as an educational override so you can move forward, but there is one small refinement to consider. The filtering logic still re-runs if a user only types spaces, because the appliedQuery dependency changes even when the trimmed text does not. To perfect this, you could introduce another useMemo to store the trimmed and lowercased query, and use that as the dependency for your main filtering logic. This would ensure the filter only runs when the actual search term changes.
Overall, this is a fantastic submission. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const filteredPeople = useMemo(() => { | ||
| return people.filter(person => | ||
| person.name.toLowerCase().includes(appliedQuery.trim().toLowerCase()), | ||
| ); | ||
| }, [appliedQuery, people]); |
There was a problem hiding this comment.
This implementation doesn't fully satisfy checklist item #1: "make sure that filter won't be called if user entered spaces only".
Currently, if appliedQuery changes from ' ' to ' ', this useMemo callback re-runs because its dependency appliedQuery has changed, even though the trimmed result is the same.
To prevent this, you could introduce another useMemo to compute the trimmed and lowercased query. The main filtering useMemo would then depend on this new memoized value, ensuring it only re-runs when the actual search term changes.
2 participants
DEMO LINK