Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the initial implementation! You've correctly used debounce for filtering and managed the state for the selected person. However, changes are needed because several core requirements are not met. The main issue is that the logic is inside the App component instead of a reusable <Autocomplete> component.
Please refactor your code to address the following:
- Create a reusable
<Autocomplete>component. This will allow you to implement the other requirements. - Accept props for
delayandonSelected. Thedelayshould be customizable, and theonSelectedcallback should be called with the selected person. - Show all suggestions on focus. The list should appear when the input is focused, even if it's empty.
- Use stable list keys. Instead of the array index, use a unique identifier from the data (like
person.name) for thekeyprop to follow React best practices [CHECKLIST ITEM #3].
✨ 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
| export const App: React.FC = () => { | ||
| const { name, born, died } = peopleFromServer[0]; | ||
| // const { name, born, died } = peopleFromServer[0]; | ||
| const delay: number = 300; |
There was a problem hiding this comment.
The delay is hardcoded here. The task requires it to be a customizable prop with a default value of 300ms. This suggests that the autocomplete logic should be in a component that can receive props.
src/App.tsx
Outdated
| if (value === '') { | ||
| setShowList(false); | ||
| setSelectedPerson(null); | ||
| } |
There was a problem hiding this comment.
This logic hides the suggestion list when the input becomes empty, which contradicts the requirement to 'show the list of all people when input is focused but empty'. Also, setSelectedPerson(null) on line 52 is redundant, as it's already called on line 48.
src/App.tsx
Outdated
| if (people.length === 0) { | ||
| setHasMatchedError(true); | ||
| } |
There was a problem hiding this comment.
This condition checks the people state before the debounced filterSuggestions function has had a chance to run and update it. This check is based on stale data and can lead to incorrect behavior. The logic for setting hasMatchedError is already correctly handled within filterSuggestions.
src/App.tsx
Outdated
| } | ||
| }; | ||
|
|
||
| const handleSelectedPerson = (person: Person) => { |
There was a problem hiding this comment.
According to the requirements, the selected person should be passed to an onSelected callback prop. This function is missing the call to that prop.
src/App.tsx
Outdated
| <div | ||
| className="dropdown-item" | ||
| data-cy="suggestion-item" | ||
| key={idx} |
There was a problem hiding this comment.
This violates checklist item #3: 'don't generate key on render'. Using the array index as a key is an anti-pattern in React, as it can cause performance and state issues when the list items are reordered. You should use a unique and stable identifier from the person object itself, for example person.name if it's unique.
…stable key , show all suggestions
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on refactoring the code into a reusable Autocomplete component! The structure is solid. However, some changes are needed before this can be approved.
The main issue is a race condition that causes all integration tests to fail. When you click a suggestion, the input's onBlur event hides the list before the click is registered, preventing selection. You can fix this by delaying the setShowList(false) call in your onBlur handler, for example, by wrapping it in a setTimeout.
Additionally, please address these points:
- The logic for showing the "No matching suggestions" message in
onChangeuses stale state and should be removed, as this is already handled correctly infilterSuggestions. - The event handler
onChangeshould be renamed tohandleChangeto follow the project's naming conventions [CHECKLIST ITEM #4]. - Hiding the list when the input is cleared goes against the requirement to show all options when the input is focused but empty.
Once these items are fixed, the component should be working perfectly. Keep up the great effort!
✨ 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
| ); | ||
|
|
||
| // Input onChange function | ||
| const onChange = (e: ChangeEvent<HTMLInputElement>) => { |
There was a problem hiding this comment.
This violates checklist item #4: 'follow [these] naming conventions for methods'. According to the convention, event handler functions should be prefixed with handle. This function should be named handleChange.
| onSelected(null); | ||
|
|
||
| if (value === '') { | ||
| setShowList(false); |
There was a problem hiding this comment.
The requirements state: 'show the list of all people when input is focused but empty'. Hiding the list here when the user clears the input goes against this requirement. The list should remain open to show all available options.
| if (people.length === 0) { | ||
| setHasMatchedError(true); | ||
| } |
There was a problem hiding this comment.
This condition uses the people state from the previous render, not the result of the current filtering operation (which is debounced). This can lead to the 'No matching suggestions' message flashing incorrectly. The logic to set hasMatchedError should only be inside filterSuggestions, which has the up-to-date filtering results.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed the critical race condition from the previous review, making the component much more robust. I am approving this solution as an educational override to help you progress, but there are a couple of minor refinements to consider for a perfect user experience.
Here are the two remaining edge cases in src/components/Autocomplete/Autocomplete.tsx:
onFocusBehavior: When a user clicks back into an input that already contains text, the suggestion list incorrectly resets to show all people. The list should instead show the results filtered by the existing text.- Selection Race Condition: If a user types quickly and then clicks a suggestion, the debounced filter can still run after the selection is made. You can prevent this by canceling any pending debounced calls (e.g., using
debouncedFilter.cancel()) inside your item selection handler.
Fixing these will make your component's behavior seamless. You're very close to a complete solution, great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| onFocus={() => { | ||
| setPeople([...peopleFromServer]); | ||
| setShowList(true); | ||
| setHasMatchedError(false); | ||
| }} |
There was a problem hiding this comment.
The onFocus handler currently resets the list to show all people, regardless of what's in the input. This can be confusing if the user focuses on an input that already has text. The requirement is to show all people when the input is empty. A better user experience would be to show the filtered list if there's text. You could achieve the correct behavior by calling filterSuggestions(inputName) here and then showing the list.
| onMouseDown={e => { | ||
| e.preventDefault(); | ||
| onSelected(person); | ||
| setInputName(person.name); | ||
| setShowList(false); | ||
| }} |
There was a problem hiding this comment.
There's a potential race condition here. If a user types and then quickly clicks a suggestion before the debounce timer fires, the debounced filter function will still execute after the selection has been made. This could cause inconsistent state. To fix this, you should cancel any pending debounced calls within this handler using debouncedFilter.cancel().
<your_account>with your Github username in the DEMO LINK and add it to the PR description.