Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on implementing the core functionality of the autocomplete component. The debouncing and state management are well done. However, I am requesting a couple of changes to fully meet the requirements.
First, the filtering logic currently runs when the input consists only of spaces, which violates a checklist requirement.
- In
src/App.tsx, please trim thequerybefore filtering the people data. - In
src/components/Dropdown.tsx, the debouncedonChangehandler should prevent callingsetQueryif the input value contains only whitespace.
Second, there's a small UI bug: the "No matching suggestions" message remains visible after the dropdown is closed. This message should only be shown when the dropdown is active (isFocused) and there are no results.
Once these points are addressed, your solution will be perfect. Keep up the great work!
✨ 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
| debounce((newValue: string) => { | ||
| setQuery(newValue); | ||
| onPersonSelect(null); | ||
| }, delay), |
There was a problem hiding this comment.
This debounce callback calls setQuery even when the newValue consists only of spaces. This violates the checklist item: "make sure that filter won't be called if user enred spaces only". Calling setQuery triggers a state update and causes the filter logic in the parent component to run. You should add a condition to prevent this when the input contains only whitespace, while still allowing an empty string to reset the search.
| {people.length === 0 && ( | ||
| <div | ||
| className=" | ||
| notification | ||
| is-danger | ||
| is-light | ||
| mt-3 | ||
| is-align-self-flex-start | ||
| " | ||
| role="alert" | ||
| data-cy="no-suggestions-message" | ||
| > | ||
| <p className="has-text-danger">No matching suggestions</p> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
The visibility of the "No matching suggestions" message depends only on the length of the people array. This causes the message to remain visible even after the dropdown is closed (e.g., when the input loses focus). The message should only appear when the user is actively searching and there are no results. Consider adding checks for isFocused and if the input value is not empty to this condition.
| const [selectedPerson, setSelectedPerson] = useState<Person | null>(null); | ||
|
|
||
| const filteredPeople = useMemo(() => { | ||
| const searchTerm = query.toLowerCase() || ''; |
There was a problem hiding this comment.
The current filtering logic doesn't handle queries that consist only of spaces. According to the checklist, the filter should treat such input as an empty search. Using query.trim() before calling toLowerCase() would solve this by removing leading/trailing whitespace from the search term.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thanks for your work on this. This requires a few changes before it can be approved.
There are two main issues remaining from the last review:
- In
Dropdown.tsx, theonChangehandler trims the input value as the user types. This prevents typing spaces, making it impossible to search for multi-word names. The input should reflect exactly what the user types, so please usesetValue(e.target.value). The trimming should only happen for the debounced query used for filtering. - The "No matching suggestions" message remains visible after the input loses focus. This message should only be shown when the dropdown is active and a search yields no results. Please adjust the visibility condition to depend on the focus state, for example:
isFocused && value && people.length === 0.
Additionally, to ensure whitespace-only searches are handled correctly, please trim the query in App.tsx before filtering: const searchTerm = query.trim().toLowerCase();.
You're very close to a complete solution. Please make these adjustments, and we'll be good to go!
✨ 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
| data-cy="search-input" | ||
| value={value} | ||
| onChange={e => { | ||
| setValue(e.target.value.trim()); |
There was a problem hiding this comment.
This line trims the user's input as they type, which can lead to a confusing user experience. For instance, if the user tries to type leading spaces, they will be removed immediately. It's better to display the user's input as-is by using setValue(e.target.value). The trimming you're doing for debouncedSetQuery is correct for the filtering logic.
| </div> | ||
| </div> | ||
|
|
||
| {people.length === 0 && ( |
There was a problem hiding this comment.
This condition for displaying the 'No matching suggestions' message doesn't fully address the feedback from the previous review. The message will remain visible even after the user clicks away from the input (i.e., when the dropdown is no longer active). The visibility should depend on the focus state. A better condition would be something like isFocused && value && people.length === 0 to ensure it only shows when the dropdown is active and a search has been performed.
| const [selectedPerson, setSelectedPerson] = useState<Person | null>(null); | ||
|
|
||
| const filteredPeople = useMemo(() => { | ||
| const searchTerm = query.toLowerCase() || ''; |
There was a problem hiding this comment.
To properly handle queries that contain only whitespace, it's best to trim the query before using it for filtering. This was mentioned in the previous review. Changing this to const searchTerm = query.trim().toLowerCase(); will ensure that inputs with only spaces are treated like an empty search, showing all results.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to support your learning journey—this allows you to move forward while addressing key concepts incrementally. You've done excellent work implementing the debounce with a customizable delay, correctly handling focus-dependent UI states (like the 'No matching suggestions' message), and using Bulma-compliant structure with proper data-cy attributes and stable keys (person.slug). However, there's an important logic bug in Dropdown.tsx: the onMouseDown handler for person selection doesn’t cancel the pending debounced query update, which can cause a race condition where onPersonSelect(null) clears the user’s choice after they’ve clicked. Additionally, resetting query to an empty string on selection leads to inconsistent behavior on re-focus — instead, set query to the selected person’s name. These are valuable learning opportunities around async state management and UX consistency. Keep up the great work — your attention to detail and progress on prior feedback is really evident!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <Dropdown | ||
| people={filteredPeople} | ||
| onPersonSelect={setSelectedPerson} | ||
| delay={300} | ||
| setQuery={setQuery} | ||
| /> |
There was a problem hiding this comment.
While the props passed here are correct, there's a logic issue inside the Dropdown component that affects how they are used, leading to a bug.
-
Race Condition Bug: The
onMouseDownhandler for selecting a person inDropdown.tsxdoesn't cancel pending debounced updates. If a user types and quickly selects an item, the delayed update can fire after selection, callingonPersonSelect(null)and clearing their choice. Thelodash.debouncefunction returns an object with a.cancel()method that should be called when an item is selected to prevent this. -
UX Issue: After selection, the
queryis reset to an empty string inside theDropdowncomponent. This causes the suggestion list to show all items on re-focus, which is inconsistent with the value in the input. Thequeryshould be updated to match the selected person's name.
Please fix this logic within the Dropdown component's selection handler.
No description provided.