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 still two critical behavior issues that violate the task requirements. First, in src/App.tsx your debouncing effect currently sets debouncedQuery directly from query, so if the user types only spaces (e.g. ' '), those spaces are debounced and passed into the filter; this breaks the explicit requirement that the filter must not be called for spaces-only input [CHECKLIST ITEM #1]. You should trim the input before updating debouncedQuery and either treat spaces-only as an empty string or skip updating in that case (for example, checking if (query.trim() === '') before calling setDebouncedQuery). Second, in your selection handler in src/App.tsx, you set the input to person.slug, but the visible suggestion text is person.name, and the requirement says the selected suggestion text should be saved to the input. Please change the selection logic so that it sets query to person.name, then calls onSelected(person) and closes the dropdown. Aside from these issues, the rest of the autocomplete behavior (debounce, dropdown, and title handling) looks solid, so once you fix these points your solution should fully meet the 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
src/App.tsx
Outdated
| </div> | ||
| useEffect(() => { | ||
| const timer = setTimeout(() => { | ||
| setDebouncedQuery(query); |
There was a problem hiding this comment.
This sets the debounced query unconditionally from the raw query. If query contains only whitespace (e.g. ' ') you will pass spaces into debouncedQuery and then run the filter against that spaces-only string. That violates the requirement: "Make sure that filter won't be called if user entered spaces only. [CHECKLIST ITEM #1]". Consider trimming or checking query.trim() here and avoid updating debouncedQuery with spaces-only input (e.g. setDebouncedQuery(query.trim() ? query : '') or skip update when query.trim() === '').
src/App.tsx
Outdated
| data-cy="suggestion-item" | ||
| key={person.name} | ||
| onClick={event => { | ||
| setQuery(person.slug); |
There was a problem hiding this comment.
On selection you set the input value to person.slug. The requirement says: "Save selected suggestion text to the input on click and close the list." — the visible suggestion text is person.name, not person.slug. Set the input value to person.name so the displayed text matches the selected suggestion, then call onSelected(person) and close the list.
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 is still a direct mismatch with the testing/requirements around the QA attributes, which will cause tests to fail. In src/App.tsx, you replaced the original data-qa attributes with data-cy (for example on the search input and suggestion items), but the task explicitly states that data-qa must be present because tests rely on them. To fix this, restore the expected data-qa attributes alongside your current ones, e.g. on the input use data-qa="search-input" in addition to or instead of data-cy="search-input", and do the same for the suggestions list, each suggestion item, the "no suggestions" message, and the title element. Without these attributes, automated tests cannot reliably select elements, which is considered a critical blocker for this assignment. Once you restore the data-qa attributes while keeping the current functionality intact, your solution should be in good shape to pass the tests and meet the 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
| type="text" | ||
| placeholder="Enter a part of the name" | ||
| className="input" | ||
| data-cy="search-input" |
There was a problem hiding this comment.
This file uses data-cy attributes (for example here) but the task requirement states: "Don't remove the data-qa attributes. It is required for tests." Please restore the data-qa attributes expected by the tests (for example add data-qa="search-input" alongside or instead of data-cy="search-input", and similarly for suggestions list, suggestion items, no-suggestions message, and the title).
mateacademy-ai-mentor
left a comment
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 continue progressing while you refine the remaining details. You implemented the core Autocomplete behavior well: Bulma dropdown usage, debounced search with a configurable delay, correct selection handling, and the required data-qa attributes are all in place. There are, however, two important checklist issues to address next:
- The filter still runs when the user types only spaces — in
src/App.tsx, update theuseMemothat computesfilteredPeopleso that whendebouncedQuery === ''you immediately returnpeople(or skip calling.filter) to satisfy [CHECKLIST ITEM #1]. - When the input is focused and empty, the list may briefly show old filtered results because you rely on
debouncedQuery; adjust the logic so that an empty focused input uses the immediatequery.trim()(or clearsdebouncedQueryon focus) to show all people right away, aligning with [CHECKLIST ITEM #3].
As a minor improvement, consider using a stable unique key (like person.id if available) instead of person.name to avoid potential key collisions in the suggestions list. Overall, you’re very close—your structure and main behaviors are solid, and polishing these last details will make your Autocomplete both more robust and fully aligned with the requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const filteredPeople = useMemo(() => { | ||
| return people.filter(person => | ||
| person.name.toLowerCase().includes(debouncedQuery.toLowerCase()), | ||
| ); | ||
| }, [people, debouncedQuery]); |
There was a problem hiding this comment.
This violates checklist item #1: "Make sure that filter won't be called if user entered spaces only." The memoized filteredPeople always calls people.filter(...) using debouncedQuery. When debouncedQuery is empty (e.g., spaces-only input trimmed to ''), .includes('') will cause filter to iterate unnecessarily. Short-circuit the memo so that when debouncedQuery === '' you return people (or otherwise avoid calling .filter) to prevent running the filter for spaces-only input.
| useEffect(() => { | ||
| const timer = setTimeout(() => { | ||
| setDebouncedQuery(query.trim()); | ||
| }, delay); | ||
|
|
||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-link">Pieter de Decker</p> | ||
| </div> | ||
| return () => { | ||
| window.clearTimeout(timer); | ||
| }; | ||
| }, [query, delay]); | ||
|
|
||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-danger">Petronella de Decker</p> | ||
| </div> | ||
| return ( | ||
| <div className={`dropdown ${isOpened ? 'is-active' : ''}`}> | ||
| <div className="dropdown-trigger"> | ||
| <input | ||
| type="text" | ||
| placeholder="Enter a part of the name" | ||
| className="input" | ||
| data-cy="search-input" | ||
| data-qa="search-input" | ||
| value={query} | ||
| onChange={event => { | ||
| onSelected(null); | ||
| setQuery(event.target.value); | ||
| }} | ||
| onFocus={() => setIsOpened(true)} | ||
| onBlur={() => setTimeout(() => setIsOpened(false), 200)} |
There was a problem hiding this comment.
This violates checklist item #3: "Show the list of all people when input is focused but empty." The component relies on debouncedQuery (set inside the debounce effect) when computing suggestions, so focusing the input while it's empty may still show the previous filtered state until the debounce timeout elapses. To satisfy the requirement, use the immediate query.trim() when the input is focused and empty (or clear debouncedQuery on focus when query is empty) so the full list of people is shown immediately without waiting for the debounce.
2 participants
DEMO LINK