Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a few checklist-required behaviors are still not correctly implemented and will likely break the tests. In src/components/Autocomplete.tsx, the debounced updater is recreated on every render and the delay prop has no default of 300ms; you should set delay = 300 in the props destructuring and create a stable debounced function via useMemo/useRef, then call .cancel() in a cleanup effect. Also in Autocomplete, the filter currently runs even when the user types only spaces, and it always re-filters even when the text hasn’t actually changed; please trim() the incoming query, skip filtering when query.trim() === '', and add an early return when the trimmed query is equal to the previous currentQuery (do the comparison before calling setCurrentQuery). In src/App.tsx, the heading uses data-cy="title" but the requirements specify data-qa attributes for tests, so update it (and any similar usage) to data-qa="title" so the test suite can locate the element. These changes are all tied to explicit task description and checklist items, so they need to be addressed before the solution can be considered complete. Once you’ve fixed these, the overall structure of your Autocomplete and App components looks solid and should satisfy the remaining 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/components/Autocomplete.tsx
Outdated
| export const Autocomplete: React.FC<Props> = ({ | ||
| people, | ||
| onSelected, | ||
| delay, | ||
| }) => { | ||
| const [shownPeople, setShownPeople] = useState([...people]); | ||
| const [focused, setFocused] = useState(false); | ||
| const [currentQuery, setCurrentQuery] = useState(''); | ||
|
|
||
| const shownPeopleUpdater = debounce(setShownPeople, delay); |
There was a problem hiding this comment.
Two related problems around the debounced updater:
- The
delayprop has no default value. The task requires a default delay of 300ms (thedelayshould be customizable via props, default 300ms) — setdelay = 300in the destructuring. - The debounced function is created inline on every render (
debounce(setShownPeople, delay)), so debouncing won't work correctly across renders. Create a stable debounced function (useuseMemooruseRef) and cancel it on unmount (e.g. call.cancel()in cleanup).
These relate to the requirement that suggestions appear after some delay (debounce) and that the delay has a default value. See description for the requirement.
src/components/Autocomplete.tsx
Outdated
| const handleFilter = (query: string) => { | ||
| const queryLow = query.toLowerCase(); | ||
|
|
||
| setCurrentQuery(query); | ||
| onSelected(null); | ||
|
|
||
| shownPeopleUpdater( | ||
| [...people].filter(person => | ||
| person.name.toLowerCase().includes(queryLow), | ||
| ), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
This code calls the filtering logic even when the user types only spaces. That violates the checklist item: "- make sure that filter won't be called if user enred spaces only;". You should trim() the incoming query and avoid calling the filter/updater when query.trim() === '' (or treat it as empty and show all people without doing the filtering call). See checklist.
src/components/Autocomplete.tsx
Outdated
| const handleFilter = (query: string) => { | ||
| const queryLow = query.toLowerCase(); | ||
|
|
||
| setCurrentQuery(query); | ||
| onSelected(null); | ||
|
|
||
| shownPeopleUpdater( | ||
| [...people].filter(person => | ||
| person.name.toLowerCase().includes(queryLow), | ||
| ), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
There is no guard to avoid re-filtering when the text hasn't changed. The function sets setCurrentQuery(query) and then always schedules the filter/update. To satisfy the requirement "don't run filtering again if the text has not changed", compare the incoming query with the previous currentQuery (the value from state) and return early when they are equal (after trimming as above). Also ensure the comparison happens before updating state so you can detect unchanged text correctly.
src/App.tsx
Outdated
| <h1 className="title" data-cy="title"> | ||
| {`${currentPerson.name} (${currentPerson.born} - ${currentPerson.died})`} | ||
| </h1> | ||
| ) : ( | ||
| <h1 className="title" data-cy="title"> | ||
| No selected person |
There was a problem hiding this comment.
This violates the checklist/description item: "Don't remove the data-qa attributes. It is required for tests." The h1 uses data-cy="title" instead of a data-qa attribute — please add/replace with data-qa="title" so tests can locate the title element. You have this at both title occurrences (lines below).
There was a problem hiding this comment.
Pull request overview
This pull request implements an Autocomplete component with debounced search functionality for displaying and filtering people from a data source. The component allows users to search through a list of people with a customizable debounce delay, select a person to display their details in the app title, and see appropriate feedback when no matches are found.
Changes:
- Created new Autocomplete component with debounced filtering, focus management, and person selection
- Updated App component to integrate the Autocomplete and display selected person details
- Updated @mate-academy/scripts dependency from version 1.8.5 to 2.1.3
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/components/Autocomplete.tsx | New component implementing autocomplete dropdown with debounced search, conditional rendering of suggestions/no-results message, and event handling for focus/blur/selection |
| src/App.tsx | Integrated Autocomplete component, added state management for selected person, and updated title rendering to show selected person or default message |
| package.json | Updated @mate-academy/scripts dependency to version 2.1.3 |
| package-lock.json | Lock file updates corresponding to the dependency version change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/Autocomplete.tsx
Outdated
| onSelected, | ||
| delay, | ||
| }) => { | ||
| const [shownPeople, setShownPeople] = useState([...people]); |
There was a problem hiding this comment.
The shownPeople state is initialized with the people prop but won't update if the people prop changes after the initial render. If the people array can change during the component's lifetime, consider using useEffect to sync the state, or derive the filtered people directly from props and the current query instead of using state.
src/components/Autocomplete.tsx
Outdated
| import classNames from 'classnames'; | ||
| import debounce from 'lodash.debounce'; | ||
| import { Person } from '../types/Person'; | ||
| import { useState } from 'react'; |
There was a problem hiding this comment.
Missing React import. The component uses React.FC but React is not imported. This will cause a runtime error. Add import React from 'react'; at the top of the file, or change line 4 to import React, { useState } from 'react';.
| import { useState } from 'react'; | |
| import React, { useState } from 'react'; |
src/components/Autocomplete.tsx
Outdated
| onSelected, | ||
| delay, | ||
| }) => { | ||
| const [shownPeople, setShownPeople] = useState([...people]); |
There was a problem hiding this comment.
Unnecessary array spreading. The initial state can be set directly to people instead of [...people]. This shallow copy doesn't provide any benefit here and adds unnecessary overhead.
| const [shownPeople, setShownPeople] = useState([...people]); | |
| const [shownPeople, setShownPeople] = useState(people); |
src/components/Autocomplete.tsx
Outdated
| onSelected(null); | ||
|
|
||
| shownPeopleUpdater( | ||
| [...people].filter(person => |
There was a problem hiding this comment.
Unnecessary array spreading before filtering. The [...people] creates a shallow copy that is immediately filtered, which is redundant. The filter method already returns a new array, so you can change this to people.filter(person => ...) directly.
| [...people].filter(person => | |
| people.filter(person => |
src/components/Autocomplete.tsx
Outdated
| className=" | ||
| notification | ||
| is-danger | ||
| is-light | ||
| mt-3 | ||
| is-align-self-flex-start | ||
| " |
There was a problem hiding this comment.
Unusual className formatting with line breaks and indentation. This should be formatted on a single line or use an array approach for better readability. Consider changing to: className="notification is-danger is-light mt-3 is-align-self-flex-start"
| className=" | |
| notification | |
| is-danger | |
| is-light | |
| mt-3 | |
| is-align-self-flex-start | |
| " | |
| className="notification is-danger is-light mt-3 is-align-self-flex-start" |
src/components/Autocomplete.tsx
Outdated
| <a | ||
| onMouseDown={() => { | ||
| handleSelect(person); | ||
| }} | ||
| className={classNames({ | ||
| 'has-text-link': person.sex === 'm', | ||
| 'has-text-danger': person.sex === 'f', | ||
| })} | ||
| > | ||
| {person.name} | ||
| </a> |
There was a problem hiding this comment.
Anchor tag without href attribute used as a clickable element. This creates accessibility issues as screen readers and keyboard navigation expect anchor tags to have href attributes. Consider using a button element instead, or add href="#" with proper event handling (e.preventDefault), or add role="button" and tabIndex={0} for proper accessibility.
src/components/Autocomplete.tsx
Outdated
| export const Autocomplete: React.FC<Props> = ({ | ||
| people, | ||
| onSelected, | ||
| delay, |
There was a problem hiding this comment.
The delay prop can be undefined, but lodash.debounce will treat undefined as 0ms delay, effectively removing the debounce behavior. According to the README, the default value should be 300ms. Add a default parameter value: change line 15 to delay = 300, to ensure proper debounce behavior when the prop is not provided.
| delay, | |
| delay = 300, |
src/components/Autocomplete.tsx
Outdated
| const [focused, setFocused] = useState(false); | ||
| const [currentQuery, setCurrentQuery] = useState(''); | ||
|
|
||
| const shownPeopleUpdater = debounce(setShownPeople, delay); |
There was a problem hiding this comment.
Debounce function is recreated on every render, which can lead to memory leaks and unexpected behavior. The debounce instance should be created once and cleaned up properly. Consider using useMemo to memoize the debounced function, or better yet, use useCallback with the debounced function and clean it up in a useEffect cleanup function.
src/components/Autocomplete.tsx
Outdated
| const handleFilter = (query: string) => { | ||
| const queryLow = query.toLowerCase(); | ||
|
|
||
| setCurrentQuery(query); | ||
| onSelected(null); | ||
|
|
||
| shownPeopleUpdater( | ||
| [...people].filter(person => | ||
| person.name.toLowerCase().includes(queryLow), | ||
| ), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
According to the checklist, the filter should not be called if the user enters spaces only. Currently, entering only spaces will trigger filtering. Add a check to trim the query and return early if it's empty or contains only whitespace, e.g., if (!query.trim()) { ... }
|
|
||
| interface Props { | ||
| people: Person[]; | ||
| onSelected: (person: Person | null) => void; |
There was a problem hiding this comment.
The prop name onSelected doesn't follow React naming conventions for event handler props. According to common React conventions, event handler props should be named with 'on' followed by the verb in base form (e.g., onSelect, onChange, onClick), not past tense. Consider renaming to onSelect for consistency with React conventions.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a few critical behaviors and test-related requirements that are not met.
Critical issues that must be fixed (blockers):
- In
src/components/Autocomplete.tsx,handleInputChangestill ends up calling the debounced filter when the user types only spaces, violating the requirement to not run filtering for spaces-only input [CHECKLIST ITEM #1]. Trim thevaluefirst (const trimmed = value.trim()), and iftrimmed === '', skip calling the debounced function and instead resetshownPeopleappropriately (e.g., to the fullpeoplelist or empty depending on your intended UX). - The same handler always calls
debouncedFilter(...)even when the trimmed text has not changed from the previous trimmed query, which contradicts the requirement not to re-filter when the text is unchanged after a pause. Store the previous trimmed query in auseRefor via existing state, comparetrimmedto that previous value, and if they are equal, return early without calling the debounced filter. - Both
src/components/Autocomplete.tsxandsrc/App.tsxusedata-cyattributes (e.g., on the input, suggestions list, suggestion items, “no suggestions” message, and the title), but the tests and task explicitly requiredata-qaattributes. Replace all relevantdata-cy="..."withdata-qa="..."so the test suite can find the elements.
As minor improvements, make sure the ordering in handleInputChange is: trim → compare with previous → decide whether to call debouncedFilter(trimmed) or reset shownPeople → then update state, which keeps behavior clear and efficient. Overall your structure, debouncing setup, and selection handling look solid; once these filtering and data-qa issues are corrected, the solution should fully meet 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 handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value; | ||
|
|
||
| setCurrentQuery(value); | ||
| onSelected(null); | ||
|
|
||
| debouncedFilter(value); |
There was a problem hiding this comment.
This violates checklist item #1: "make sure that filter won't be called if user enred spaces only;" Currently the debounced filter is still invoked from handleInputChange even if the input contains only spaces. Move the trim() and early-empty check before calling the debounced function (or skip calling it when value.trim() === '') so the filter isn't called for spaces-only input.
| const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value; | ||
|
|
||
| setCurrentQuery(value); | ||
| onSelected(null); | ||
|
|
||
| debouncedFilter(value); |
There was a problem hiding this comment.
This does not satisfy 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)". The code always calls debouncedFilter(value) on every change event, so a pause with the same text will re-trigger filtering. Compare the incoming trimmed query with the previous trimmed query (store the previous in a ref or use currentQuery) and skip calling the debounced filter when they are equal to avoid unnecessary re-filtering.
src/components/Autocomplete.tsx
Outdated
| data-cy="search-input" | ||
| value={currentQuery} | ||
| onFocus={() => setFocused(true)} | ||
| onBlur={() => setFocused(false)} | ||
| onChange={handleInputChange} | ||
| /> | ||
| </div> | ||
|
|
||
| {focused && shownPeople.length > 0 && ( | ||
| <div className="dropdown-menu" role="menu" data-cy="suggestions-list"> | ||
| <div className="dropdown-content"> | ||
| {shownPeople.map(person => ( | ||
| <a | ||
| onMouseDown={() => handleSelect(person)} | ||
| key={person.slug} | ||
| className={classNames('dropdown-item', { | ||
| 'has-text-link': person.sex === 'm', | ||
| 'has-text-danger': person.sex === 'f', | ||
| })} | ||
| data-cy="suggestion-item" | ||
| style={{ cursor: 'pointer' }} | ||
| > | ||
| {person.name} | ||
| </a> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| )} | ||
|
|
||
| {focused && shownPeople.length === 0 && currentQuery !== '' && ( | ||
| <div | ||
| className="notification is-danger is-light mt-3" | ||
| role="alert" | ||
| data-cy="no-suggestions-message" |
There was a problem hiding this comment.
Tests and the task description require data-qa attributes: "Don't remove the data-qa attributes. It is required for tests." This file uses data-cy on the input, suggestions list, suggestion items and the no-suggestions message. Replace these data-cy attributes with data-qa so the test suite can locate elements.
| const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value; | ||
|
|
||
| setCurrentQuery(value); | ||
| onSelected(null); | ||
|
|
||
| debouncedFilter(value); |
There was a problem hiding this comment.
Minor correctness hint: you trim query inside the debounced callback (good), but to fully satisfy both the spaces-only and unchanged-text requirements, perform const trimmed = value.trim() in handleInputChange, compare to previous trimmed query and only call debouncedFilter(trimmed) when needed (or skip calling and directly set shownPeople to people for empty trimmed). This prevents calling the debounced filter unnecessarily and ensures correct behavior for spaces-only input.
src/App.tsx
Outdated
| <p className="has-text-danger">No matching suggestions</p> | ||
| </div> | ||
| {currentPerson ? ( | ||
| <h1 className="title" data-cy="title"> |
There was a problem hiding this comment.
This uses data-cy="title". The task description requires data-qa attributes for tests: "Don't remove the data-qa attributes. It is required for tests." Please change to data-qa="title" so the test suite can locate the element.
src/App.tsx
Outdated
| {`${currentPerson.name} (${currentPerson.born} - ${currentPerson.died})`} | ||
| </h1> | ||
| ) : ( | ||
| <h1 className="title" data-cy="title"> |
There was a problem hiding this comment.
This uses data-cy="title" in the "No selected person" branch. As above, change to data-qa="title" to satisfy the requirement: "Don't remove the data-qa attributes. It is required for tests."
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 while you refine the remaining behaviors. You did a solid job wiring up the Autocomplete with Bulma, using a debounced filter with a configurable delay, handling selection updates, and correctly rendering the title and data-qa="title" in App.tsx. The main behavior gaps are in src/components/Autocomplete.tsx: your input handler still calls the debounced filter even when the user types only spaces, which conflicts with [CHECKLIST ITEM #1] that requires not calling the filter at all for spaces-only input. To fix this, compute const trimmed = value.trim() first, and if trimmed === '' just reset shownPeople (or keep people) and do not call the debounced function. You should also compare this trimmed value with the previously trimmed query (stored in a useRef or state), return early if they’re equal to avoid re-filtering, and only then update state (setCurrentQuery and onSelected(null)) and decide whether to call the debounced filter using the trimmed string. Once you adjust the ordering and trimming logic in the input-change handler, your implementation will better match the behavioral requirements and should be more robust in tests. Overall, you’re very close—your structure and main logic are good, and these refinements will strengthen your understanding of controlled inputs and debounced search flows.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| setCurrentQuery(value); | ||
| onSelected(null); | ||
|
|
||
| debouncedFilter(value); |
There was a problem hiding this comment.
This still violates checklist item #1 exactly: "make sure that filter won't be called if user enred spaces only;" (from checklist). You call debouncedFilter(value) here even when value may be spaces-only. Although the debounced filter trims internally, the requirement is to avoid calling the filter at all for spaces-only input. Trim the input first and, if the trimmed string is empty, reset shownPeople (or keep people) and do not invoke the debounced function.
| const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value; | ||
|
|
||
| if (value.toLowerCase().trim() === currentQuery.toLowerCase().trim()) { | ||
| setCurrentQuery(value); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| setCurrentQuery(value); | ||
| onSelected(null); | ||
|
|
||
| debouncedFilter(value); | ||
| }; |
There was a problem hiding this comment.
Reorder the input-change logic to: 1) compute const trimmed = value.trim(), 2) compare trimmed with the previous trimmed query (use a ref or stored trimmed value) and return early if equal so you don't re-run filtering, 3) decide whether to call the debounced filter (pass the trimmed string) or reset shownPeople when trimmed is empty, and only then 4) update state (setCurrentQuery and onSelected(null)). Right now the handler sets state and calls debouncedFilter with the untrimmed value, and it can call the debounced filter for spaces-only — adjust ordering and pass trimmed query to satisfy the requirements.
No description provided.