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 a couple of critical issues with your useEffect hooks that impact performance and user experience.
First, the useEffect that focuses the input is missing a dependency array ([]). This causes it to run on every single render, constantly refocusing the input and making other interactions difficult. This also violates our checklist item on DOM interactions [CHECKLIST ITEM #2].
Second, the useEffect for debouncing includes lasrQuery in its dependency array, which causes it to re-run unnecessarily since it's set within that same effect. It should only depend on query and delay.
Please also fix the typos in your state variables (lasrQuery and setLAstQuery). You've built the main features correctly, and with these fixes, your component will be working perfectly!
✨ 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 [query, setQuery] = useState(''); | ||
| const [isActive, setIsActive] = useState(false); | ||
| const [lasrQuery, setLAstQuery] = useState(''); |
There was a problem hiding this comment.
There are a few typos in your variable names which make the code harder to read. For example, lasrQuery and setLAstQuery should likely be lastQuery and setLastQuery.
| useEffect(() => { | ||
| field.current?.focus(); | ||
| }); |
There was a problem hiding this comment.
This useEffect hook is missing a dependency array. This will cause it to run after every single render, constantly refocusing the input field and potentially preventing users from interacting with other parts of the application. To ensure this effect runs only once when the component mounts, you should provide an empty dependency array ([]).
|
|
||
| useEffect(() => { | ||
| const timerId = window.setTimeout(() => { | ||
| const normalaizeQuery = query.trim(); |
There was a problem hiding this comment.
There's a typo here in normalaizeQuery. It should probably be normalizedQuery.
| return () => { | ||
| clearTimeout(timerId); | ||
| }; | ||
| }, [query, delay, lasrQuery]); |
There was a problem hiding this comment.
Including lasrQuery in the dependency array causes this effect to re-run whenever it's updated. While your if condition prevents an infinite loop, the effect should ideally only run in response to changes in query or delay. It's a good practice to avoid including state variables in a dependency array if they are set within that same effect.
| )} | ||
| </div> | ||
|
|
||
| {filteredPeople.length === 0 && lasrQuery.length !== 0 && ( |
There was a problem hiding this comment.
The 'No matching suggestions' message remains on screen even after the input field loses focus, as its visibility isn't tied to the isActive state. For a better user experience, this message should typically disappear when the autocomplete dropdown is closed. Consider adding isActive to this condition.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing most of the feedback from the previous review! The component is very close to being ready, but I'm requesting one important change to finalize the debouncing logic.
In src/components/PeopleAutocomplete.tsx, the useEffect hook for debouncing includes lastQuery in its dependency array. Since setLastQuery is called inside this effect, it triggers an unnecessary re-render and causes the effect to run a second time. This undermines the performance benefit of debouncing and violates the requirement to not re-run filtering if the text hasn't changed.
To fix this, please remove lastQuery from the dependency array. The effect should only depend on query and delay to work as intended. After this change, the component will be perfect!
✨ 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
| return () => { | ||
| clearTimeout(timerId); | ||
| }; | ||
| }, [query, delay, lastQuery]); |
There was a problem hiding this comment.
The dependency array for this useEffect hook shouldn't include lastQuery. Including it causes the effect to run an extra time after setLastQuery is called inside it. This is inefficient and not the intended way to implement debouncing.
This was also mentioned in the previous review. The dependency array should only contain query and delay to ensure the effect only runs when the user's input changes.
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 submission! I am approving your solution. You've done a fantastic job addressing the feedback from the previous review, and the debouncing logic is now implemented perfectly. The component is robust, handles edge cases well, and the code is clean and well-structured. You've successfully met all the project requirements. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
The test task has already been approved.
[CODE: AAP]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
2 participants
No description provided.