Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a few critical requirements from the task and checklist are not yet fully met, even though the overall behavior is close.
Critical issues that must be fixed:
- In
src/components/Autocomplete.tsx, the inputonChangehandler currently calls the debouncedapplyQuerywith the raw value, so a query of only spaces still triggers filtering, which violates the requirement that filtering must not run for spaces-only input [CHECKLIST ITEM #1]; trim the value and skip calling the debounced setter when the trimmed value is empty (treat it as an empty query instead). - The component attaches global document listeners (
document.addEventListener/document.removeEventListener) to detect outside clicks, which goes against the guideline to avoid direct DOM interaction [CHECKLIST ITEM #2]; prefer a React-driven approach such as relying ononBlur/onFocuson a wrapper or using an existing React outside-click hook instead of touchingdocumentdirectly in this component. - The
delayprop is required in theAutocompleteprops and has no default, but the task explicitly requires a default of 300ms; makedelayoptional in the type and setdelay = 300in the destructured props so the component behaves correctly even when the parent omits it. - The
"No matching suggestions"message is rendered wheneverfilterPeople.length === 0, even when the dropdown is closed; update the rendering condition so it only appears when the menu is open (e.g.isOpenMenu && filterPeople.length === 0) as part of the suggestions UI.
Minor but important improvements:
- The input change handler is typed as a
FocusEventinstead of aChangeEvent, so update its signature to useReact.ChangeEvent<HTMLInputElement>for correctness and clearer intent, and consider trimming the query once upfront before filtering. Overall, you’ve implemented most of the core interactions correctly (debounced suggestions, selection, clearing on change), and once these issues are addressed your solution will align well with the task requirements.
✨ 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 handleQueryChange = (event: React.FocusEvent<HTMLInputElement>) => { | ||
| setInputValue(event.target.value); | ||
| applyQuery(event.target.value); | ||
| onSelected(null); | ||
| }; | ||
|
|
||
| const filterPeople = useMemo(() => { | ||
| return people.filter(person => { | ||
| return person.name | ||
| .trim() | ||
| .toLocaleLowerCase() | ||
| .includes(appliedQuery.toLocaleLowerCase()); | ||
| }); | ||
| }, [appliedQuery, people]); |
There was a problem hiding this comment.
This code calls applyQuery with the raw input value and then filters using appliedQuery. This allows filtering to run when the user types only spaces. The checklist requires: "- make sure that filter won't be called if user enred spaces only;" Consider trimming the input and avoiding calling the debounced setter when the trimmed value is empty (or explicitly treat a spaces-only input as an empty query).
| useEffect(() => { | ||
| const handleClickOutside = (event: MouseEvent) => { | ||
| if ( | ||
| menuRef.current && | ||
| !menuRef.current.contains(event.target as Node) && | ||
| inputRef.current && | ||
| !inputRef.current.contains(event.target as Node) | ||
| ) { | ||
| setIsOpenMenu(false); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('mousedown', handleClickOutside); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('mousedown', handleClickOutside); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
You're attaching a document-level event listener to detect outside clicks. The checklist states: "- don't interact with DOM directly, use React as much as possible;" Adding/removing listeners on document is direct DOM interaction. Prefer using React events (for example, onBlur/onFocus handling on the input/wrapper or managing focus within React) or a React-friendly outside-click hook that still avoids manipulating document directly from component logic.
| export const Autocomplete: React.FC<Props> = ({ | ||
| people, | ||
| delay, | ||
| onSelected, | ||
| }) => { |
There was a problem hiding this comment.
The task requires: "the delay should be customizable via props (default value is 300ms)" but the component's Props mark delay as required and the destructured props don't provide a default. Provide a default value (for example: ({ people, delay = 300, onSelected })) or make delay optional in the props type with a default of 300ms inside the component.
| {filterPeople.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 No matching suggestions message is rendered whenever filterPeople.length === 0, even if the suggestions menu is closed. The requirement says: "show No matching suggestions message if there are no people containing the entered text;" Make the message conditional on the menu being open (e.g., isOpenMenu && filterPeople.length === 0) so the message appears only as part of the open suggestions UI.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this can be considered complete, mainly around the input handling and when filtering is triggered. In src/components/Autocomplete.tsx, the input onChange handler is currently typed as a focus event; this must be changed to React.ChangeEvent<HTMLInputElement> so that event.target.value is correctly typed and the handler matches the required signature. Also in that same handler, you are passing the raw input value directly into the debounced query setter, which means an input of only spaces will still trigger debounced filtering; instead, trim the value, and when the trimmed value is empty, immediately set the applied query to '' and avoid calling the debounced function at all. Additionally, the task requires that filtering is not re-run when the text has not changed: before scheduling the debounced call, compare the (trimmed) new value to the current appliedQuery and skip calling the debounced function when they are equal. These three issues are considered critical because they explicitly violate the task/checklist requirements even though most other behaviors (debounce, selection, showing all people on focus, and “No matching suggestions” visibility) are implemented correctly. Once you fix the event type and adjust the change handler logic to trim input, ignore spaces-only queries, and guard against unchanged queries, your solution will fully align with the specification—you're very close, and these focused tweaks will solidify an otherwise strong implementation.
✨ 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
| [setAppliedQuery, delay], | ||
| ); | ||
|
|
||
| const handleQueryChange = (event: React.FocusEvent<HTMLInputElement>) => { |
There was a problem hiding this comment.
The input change handler is typed as a focus event. Update it to use the required type: "The input onChange handler must be typed as React.ChangeEvent<HTMLInputElement> (not as a focus event)". This will correctly type event.target.value and match the task requirement.
| setInputValue(event.target.value); | ||
| applyQuery(event.target.value); |
There was a problem hiding this comment.
This currently calls the debounced setter with the raw value, so a query of only spaces will trigger filtering. Fix per the checklist: "- make sure that filter won't be called if user enred spaces only;" — trim the input and, when the trimmed value is empty, treat it as an empty query (e.g. set setAppliedQuery('') synchronously) and do NOT call the debounced setter with spaces-only input.
|
|
||
| const handleQueryChange = (event: React.FocusEvent<HTMLInputElement>) => { | ||
| setInputValue(event.target.value); | ||
| applyQuery(event.target.value); |
There was a problem hiding this comment.
There is no guard to avoid re-running filtering when the text hasn't changed. The task states: "- don't run filtering again if the text has not changed (a pause in typing happened when the text was the same as before)". Before calling the debounced setter, compare the new value (or its trimmed form) to the current appliedQuery and skip calling applyQuery if they are equal.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing, even though there are still a couple of behaviors to refine. You did a nice job implementing the Bulma dropdown structure, debounced searching, selection handling, and correctly updating the h1 title from the selected person. The main remaining issue is in src/components/Autocomplete.tsx, in the handleQueryChange logic used by the input’s onChange (around the middle of the file): when the user types only spaces, you still call the debounced query setter with the raw newValue, which violates the requirement that the filter must not run for a spaces-only query [CHECKLIST ITEM #1]; instead, trim the value, and if the trimmed value is empty, immediately call setAppliedQuery('') and return without scheduling the debounce. In that same function, the debounced setter should receive the trimmed/normalized text rather than the raw input so appliedQuery never stores just whitespace. Also in handleQueryChange, you currently clear the selected person (onSelected(null)) on every keystroke; to match the requirement more precisely, only clear the selection after you’ve compared the trimmed new value to the current appliedQuery and confirmed that the effective query has actually changed, so brief pauses or unchanged text don’t wipe the selection unexpectedly. Overall, your structure and adherence to most of the checklist is strong, and tightening up these small edge cases will make your Autocomplete feel much more robust and polished.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| setInputValue(newValue); | ||
| onSelected(null); | ||
|
|
||
| if (normalizedNewValue === trimAppliedQuery) { | ||
| return; |
There was a problem hiding this comment.
You're clearing the selected person unconditionally here. According to the requirements, the selected person should be cleared when the input value actually changes (so the title updates to 'No selected person'). Move the onSelected(null) call so it only runs when the trimmed/normalized new value differs from the current applied query (i.e., after the equality check), otherwise a pause/unchanged text will still clear the selection unexpectedly.
| const newValue = event.target.value; | ||
| const normalizedNewValue = newValue.trim().toLowerCase(); | ||
|
|
||
| setInputValue(newValue); | ||
| onSelected(null); | ||
|
|
||
| if (normalizedNewValue === trimAppliedQuery) { | ||
| return; | ||
| } | ||
|
|
||
| applyQuery(newValue); |
There was a problem hiding this comment.
This code currently calls the debounced query setter with the raw newValue and will schedule filtering when the user types spaces-only. This violates the checklist item: "make sure that filter won't be called if user enred spaces only;". Instead, when normalizedNewValue is empty (spaces or empty string) you should immediately set appliedQuery to '' (not via debounce) and return. Also, when scheduling the debounced setter, pass the trimmed/normalized value (not the raw newValue) so appliedQuery stores the normalized text rather than whitespace.
https://ruslanv23.github.io/react_autocomplete/