Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this solution can be considered complete and aligned with the task requirements.
The most important issue is that the debounced filter is still called when the user types only spaces, which violates the explicit checklist item about ignoring spaces-only input; in src/App.tsx, update your input change handler to use const trimmed = event.target.value.trim() and skip calling the debounced applyQuery when trimmed === ''. Related to debounce, you also need to avoid redundant filtering by checking whether the new (trimmed) value is equal to the current appliedQuery before invoking the debounced updater, so the filter is not re-run when the text hasn’t actually changed. The visibility of suggestions and the “No matching suggestions” message currently depend on the immediate query while filteredPeople is derived from the debounced appliedQuery, which can show an empty list or the “No matching suggestions” message too early; base both the dropdown visibility (especially when the input is focused and empty) and the “no results” message on the debounced/applied value so they update only after the debounce delay. In addition, the debounced function is created without any cleanup; it’s safer to create it in a useMemo/useEffect and call .cancel() in the cleanup function to prevent stale calls after unmount or delay changes. Finally, make sure that selecting a person explicitly hides the dropdown (e.g., by updating your isFocused/open state) and that you are using the required data-qa attributes instead of any alternative like data-cy, as tests often depend on those.
Once you address these behavioral issues, your autocomplete will more closely match the spec, and the solid structure you already have with Bulma markup, debouncing, and selection handling will really shine.
✨ 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.ChangeEvent<HTMLInputElement>) => { | ||
| setQuery(event.target.value); | ||
| setSelectedPerson(null); | ||
| applyQuery(event.target.value); |
There was a problem hiding this comment.
This calls the debounced update unconditionally with the raw input value. This violates the checklist item: "- make sure that filter won't be called if user enred spaces only;" — consider trimming the input and avoid invoking applyQuery when event.target.value.trim() === '' (or handle spaces-only input separately) so the filter won't run for spaces-only input. Also check whether the new value equals the current appliedQuery before calling the debounced updater to avoid re-filtering when the text hasn't changed.
|
|
||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-danger">Elisabeth Hercke</p> | ||
| {isFocused && filteredPeople.length > 0 && ( |
There was a problem hiding this comment.
The suggestions list visibility depends on filteredPeople (which is derived from the debounced appliedQuery) and isFocused. Because filteredPeople uses the debounced value, focusing an input that is immediately empty may not reliably show the full list if appliedQuery is still stale. The requirement says: "show the list of all people when the input is focused but empty" — consider using the immediate query (trimmed) to decide to show all people when focus occurs, instead of waiting for the debounced value.
|
|
||
| <div | ||
| className=" | ||
| {query && filteredPeople.length === 0 && ( |
There was a problem hiding this comment.
The No matching suggestions message is shown based on the immediate query ({query && filteredPeople.length === 0}) while filteredPeople is computed from the debounced appliedQuery. This can show the message too early (before debounce applies) or incorrectly when the debounced value is still the previous text. To match the requirement "show No matching suggestions message if there are no people containing the entered text" you should base this condition on the debounced/applied value (or otherwise coordinate timing) so the message appears after the debounce filtering completes.
| const [selectedPerson, setSelectedPerson] = useState<Person | null>(null); | ||
| const [isFocused, setIsFocused] = useState(false); | ||
|
|
||
| const applyQuery = useCallback(debounce(setAppliedQuery, delay), [delay]); |
There was a problem hiding this comment.
The debounced updater is created with useCallback(debounce(setAppliedQuery, delay), [delay]) but there is no cleanup to cancel pending debounced calls on unmount or when delay changes. It is safer to create the debounced function inside a useMemo/useEffect and call .cancel() in cleanup to avoid possible stale calls or memory leaks.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am asking for revisions because a few core behavioral requirements are still not fully met and will likely cause tests to fail. In src/App.tsx, the input change handler currently calls the debounced updater for any non-empty trimmed value; you need to compare the new trimmed value to appliedQuery and skip calling the debounced applyQuery when they are equal so filtering is not re-run on pauses with unchanged text. Also in src/App.tsx, when a suggestion is clicked you set the input value and selectedPerson, but you do not close the dropdown; ensure you set isFocused to false (or otherwise hide the dropdown) as part of the click handler so the list closes immediately after selection. The visibility of the “No matching suggestions” message is currently driven by (appliedQuery || query), which means it can appear before the debounce has applied; instead, base both the dropdown visibility and the “no results” message solely on the debounced appliedQuery so the UI only reflects the filtered state after the delay. Across your JSX, required testing attributes use data-cy (e.g. data-cy="search-input") instead of the specified data-qa names, so you should rename them to data-qa to align with the task and allow tests to locate elements. As minor improvements, consider creating the debounced function with useMemo or similar for clarity, and double-check that person.name is unique if you rely on it as a React key. Overall your solution is very close—once these behavioral and attribute fixes are in place, it should fully satisfy 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 trimmed = value.trim(); | ||
|
|
||
| if (!trimmed) { | ||
| setAppliedQuery(''); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| applyQuery(trimmed); |
There was a problem hiding this comment.
The handler always calls the debounced updater for any non-empty trimmed value. This violates the requirement “don't run filtering again if the text has not changed (a pause in typing happened when the text was the same as before)”. Add a check comparing the new trimmed value to the current appliedQuery and skip calling applyQuery if they are equal (so filtering isn't re-run unnecessarily).
| onMouseDown={() => { | ||
| setSelectedPerson(person); | ||
| setQuery(person.name); | ||
| setAppliedQuery(person.name); | ||
| onSelected?.(person); | ||
| }} |
There was a problem hiding this comment.
When a suggestion is selected you set the input value and selectedPerson, but you don't explicitly close the dropdown. The spec requires you to “save selected suggestion text to the input on click and close the list;” ensure you call setIsFocused(false) (or otherwise hide the dropdown) when selecting an item so the list actually closes immediately.
|
|
||
| <div | ||
| className=" | ||
| {(appliedQuery || query) && filteredPeople.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> | ||
| 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 suggestions’ message visibility uses (appliedQuery || query) && filteredPeople.length === 0, which can show the message too early (before debounce applies) because it depends on the immediate query. To match the expected behavior, drive the “No matching suggestions” message (and dropdown visibility) from the debounced/applied value (appliedQuery) so the UI updates only after debounce.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved as an educational override so you can keep progressing while you learn, even though there’s one important testing issue to fix.
You’ve done a strong job on the core behavior: the debounced search using lodash.debounce, showing all people when the input is focused and empty, clearing the selected person when the query changes, and correctly selecting a person with onMouseDown and calling onSelected all match the task requirements. The main blocking issue is that in src/App.tsx you used data-cy instead of the required data-qa attributes on multiple elements: the title (data-cy="title"), the input (data-cy="search-input"), the suggestions list (data-cy="suggestions-list"), each suggestion item (data-cy="suggestion-item"), and the no-results message (data-cy="no-suggestions-message"). Tests look specifically for data-qa="title", data-qa="search-input", data-qa="suggestions-list", data-qa="suggestion-item", and data-qa="no-suggestions-message", so you should rename each of these attributes accordingly so automated tests can locate the elements. As a minor improvement, consider using a guaranteed-unique field (like an id) for the React key on each suggestion item if available, to avoid potential issues when two people share the same name.
Overall, your implementation shows good attention to behavior details (including ignoring spaces-only input [CHECKLIST ITEM #1] and avoiding direct DOM access [CHECKLIST ITEM #2]), and with the small data-qa attribute fix your solution would be fully test-ready—nice work, keep building on this foundation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| return ( | ||
| <div className="container"> | ||
| <main className="section is-flex is-flex-direction-column"> | ||
| <h1 className="title" data-cy="title"> |
There was a problem hiding this comment.
The task and tests require data-qa attributes, but this element uses data-cy="title". Rename it to data-qa="title" so tests can locate the element.
| @@ -19,55 +64,56 @@ export const App: React.FC = () => { | |||
| placeholder="Enter a part of the name" | |||
| className="input" | |||
| data-cy="search-input" | |||
There was a problem hiding this comment.
The input uses data-cy="search-input". The instructions and tests require data-qa attributes. Rename to data-qa="search-input".
| <div | ||
| className="dropdown-menu is-active" | ||
| role="menu" | ||
| data-cy="suggestions-list" |
There was a problem hiding this comment.
The suggestions list uses data-cy="suggestions-list". Rename to data-qa="suggestions-list" to match the task/test requirements.
| <div | ||
| key={person.name} | ||
| className="dropdown-item" | ||
| data-cy="suggestion-item" |
There was a problem hiding this comment.
Each suggestion item uses data-cy="suggestion-item". Change it to data-qa="suggestion-item" as required by the task.
| <p className="has-text-danger">No matching suggestions</p> | ||
| </div> | ||
| role="alert" | ||
| data-cy="no-suggestions-message" |
There was a problem hiding this comment.
The no-results notification uses data-cy="no-suggestions-message". Update it to data-qa="no-suggestions-message" so tests can find it.
DEMO LINK