Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several core requirements from the task description are not yet implemented, which would block full task completion. The biggest gap is the absence of a dedicated Autocomplete component with the required API (e.g. people, delay, onSelected), while the current logic lives directly in App.tsx; this violates the explicit requirement to implement that component. Debounce behavior is also missing: suggestions are shown immediately on every keystroke, and there is no configurable delay prop with a default of 300 ms, nor logic to avoid re-running filtering when the (trimmed) text hasn’t actually changed. Additionally, [CHECKLIST ITEM #1] is not fully respected in App.tsx: handleInputChange stores values consisting only of spaces into query, which still triggers the filtering hook and related effects instead of ignoring pure-space input or normalizing it before updating state.
To fix this, you should: 1) create an Autocomplete component that encapsulates the input, dropdown, and list, accepts a delay?: number = 300 and onSelected, and uses PeopleList/useFilter inside; 2) implement debouncing (e.g. via setTimeout in a useEffect) so filtering only happens after the delay and only when the trimmed query changes; and 3) adjust handleInputChange (or the new component’s input handler) to skip updating the filter query when the new value is spaces-only (for example by maintaining rawInput vs queryForFilter or checking value.trim()). Once these are in place, your existing good practices—stable keys, no direct DOM manipulation, proper Bulma usage, and correct “No matching suggestions” handling—will fit into a complete, spec-compliant solution. Keep going; the structure is solid, you mainly need to add the missing component and input-handling refinements.
✨ 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 = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| setQuery(event.target.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". The current check in useFilter (if (!query.trim())) will treat a query consisting of spaces as empty and return all people, which is correct for showing all when input is empty, but the checklist requires not to call the filter when user entered spaces only. However, in App.tsx, handleInputChange always sets query to event.target.value (including spaces) which will cause the hook dependency to run. To satisfy the checklist, you should normalize or ignore pure-space input before updating query (for example, check if (event.target.value.trim() === '' && event.target.value !== '') and avoid updating query or handle differently).
| data-cy="search-input" | ||
| value={query} | ||
| onChange={handleInputChange} | ||
| onFocus={() => setShow(true)} |
There was a problem hiding this comment.
This violates checklist item #2: "don't interact with DOM directly, use React as much as possible." I don't see direct DOM interactions in these files, but App currently uses onFocus={() => setShow(true)} which is fine. No DOM direct manipulation found — no action required here unless other files do direct DOM access.
|
|
||
| const filteredPeople = useFilter(data, query); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This violates checklist item #3: "don't generate key on render." In PeopleList you use key={person.slug} which is stable and acceptable. No dynamic/generated key on render is present, so no change needed for keys.
| setQuery(event.target.value); | ||
| setSelectedPerson(null); | ||
| setShow(true); | ||
| }; |
There was a problem hiding this comment.
This may violate checklist item #4 (naming conventions). Event handler props are named handleInputChange and handleSelect which follow common conventions; no violation found here.
| interface PeopleListProps { | ||
| people: Person[]; | ||
| onSelect?: (person: Person) => void; | ||
| } |
There was a problem hiding this comment.
This file doesn't prevent filtering when the input contains only spaces. The checklist requires: "make sure that filter won't be called if user enred spaces only". In useFilter, the condition if (!query.trim()) { return people; } will treat a spaces-only query as empty and return all people (which avoids running .filter), but higher-level requirement is to not call filter when user entered spaces only. The current useFilter implementation avoids filtering, so it's OK here. However, in App.tsx handleInputChange sets setQuery(event.target.value) directly for spaces, which will cause useFilter to run but will short-circuit; this is acceptable. No change required for this specific rule.
|
|
||
| export const useFilter = (people: Person[], query: string) => { | ||
| return useMemo(() => { | ||
| if (!query.trim()) { |
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'. The current filter hook calls query.trim() and returns all people when trimming yields empty — that prevents filtering on spaces-only input in useFilter. However, the App's handleInputChange sets setQuery(event.target.value) directly and clears selectedPerson; so spaces-only input becomes a non-empty query in state until useFilter's trim check treats it as empty. This behavior meets the requirement (filter won't be run for spaces-only). No change needed.
| person.name.toLowerCase().includes(query.toLowerCase()), | ||
| ); | ||
| }, [people, query]); | ||
| }; |
There was a problem hiding this comment.
Missing debounce requirement: The description requires suggestions appear after some delay in typing (debounce) with a customizable delay prop (default 300ms). Neither App nor useFilter implements debouncing or accepts a delay prop. Implement debounce logic (e.g., in a custom hook or in App) that delays updating the query passed to the filter, and expose a prop to configure the delay with default 300ms. Without this, the core functional requirement is not satisfied.
| person.name.toLowerCase().includes(query.toLowerCase()), | ||
| ); | ||
| }, [people, query]); | ||
| }; |
There was a problem hiding this comment.
Missing 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)'. There's no logic preventing repeated filtering on identical query pauses — currently useFilter depends on query and people, so if query doesn't change, filter won't rerun, but because App sets query from the raw input on every onChange, repeated identical values won't happen. However the debounce behavior that's missing could cause redundant filtering; implement debounce and ensure you compare previous filtered text to avoid re-filtering when the trimmed query equals previous trimmed query.
| @@ -0,0 +1,14 @@ | |||
| import { useMemo } from 'react'; | |||
| import { Person } from '../types/Person'; | |||
There was a problem hiding this comment.
Missing requirement: 'Implement an Autocomplete component that will suggest people matching an entered text.' There is no separate Autocomplete component in the submitted files; App implements the dropdown directly and uses PeopleList and useFilter. You must implement a dedicated Autocomplete component with props (people, delay, onSelected, etc.) per the spec.
| @@ -0,0 +1,14 @@ | |||
| import { useMemo } from 'react'; | |||
| import { Person } from '../types/Person'; | |||
|
|
|||
There was a problem hiding this comment.
Requirement: 'The delay should be customizable via props (default value is 300ms)'. No component accepts a delay prop or provides default. Add delay prop and default value.
|
I fixed the errors. Please double-check. |
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a couple of explicit checklist requirements are still not met, which makes the implementation incomplete.
Critical issues that must be fixed:
- The debounce effect in
src/components/Autocomplete.tsxstill triggers filtering when the user types spaces-only, violating the requirement “make sure that filter won't be called if user entered spaces only” [CHECKLIST ITEM #1]. Right now you doconst trimmed = rawInput.trim(); setQuery(trimmed);after the delay; whenrawInputis only spaces,trimmedbecomes''andsetQuery('')causes the filter hook to run and return all people. Fix hint: detect this case, e.g. ifrawInput.length > 0 && trimmed === ''then skip callingsetQueryso the filter is not re-run for whitespace-only input. - The same debounce effect calls
setQuery(trimmed)on every timeout even when the trimmed value hasn’t changed, violating “don't run filtering again if the text has not changed (a pause in typing happened when the text was the same as before)”. Fix hint: compare the new trimmed value to the currentquery(or track the last trimmed value in state/ref) and only callsetQuerywhen they differ.
Minor/behavioral note (not blocking, but worth checking): you already clear the selected person on every input change via onSelected(null) in handleChange, which satisfies the requirement that changing the input clears the selection; just ensure this still happens even when you decide to ignore spaces-only for filtering, so selection is cleared on any user modification. Overall, the structure and most behaviors are correct (Bulma dropdown, default delay, showing all people when query is empty, stable keys, no direct DOM access), and once you adjust the debounce logic to handle whitespace-only input and avoid redundant filtering, your solution will meet 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
| useEffect(() => { | ||
| const trimmed = rawInput.trim(); | ||
|
|
||
| const timer = setTimeout(() => { | ||
| setQuery(trimmed); // даже пустая строка → покажет всех |
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;"
In Autocomplete.tsx the debounce effect sets const trimmed = rawInput.trim(); then always calls setQuery(trimmed) after delay, which means if user types only spaces (rawInput consists of spaces) trimmed becomes empty string and setQuery('') will cause the filter hook to run and return all people. The requirement asks that filter shouldn't be called if user entered spaces only — you should avoid updating query (or avoid triggering filter) when the input is spaces-only. Consider skipping setQuery when rawInput contains only whitespace (e.g. if rawInput.length > 0 && trimmed === '').
| useEffect(() => { | ||
| const trimmed = rawInput.trim(); | ||
|
|
||
| const timer = setTimeout(() => { | ||
| setQuery(trimmed); // даже пустая строка → покажет всех | ||
| }, delay); | ||
|
|
||
| return () => clearTimeout(timer); |
There was a problem hiding this comment.
This violates the checklist instruction: "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 current debounce effect depends only on [rawInput, delay] and always sets query to trimmed value after each timer. If rawInput (after trimming) equals the previous query, you still call setQuery which is unnecessary and may re-run filtering. Add a guard to only call setQuery when trimmed !== query (or track previous trimmed value) to avoid redundant filtering runs.
| const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value; | ||
|
|
||
| setRawInput(value); | ||
| setShow(true); | ||
|
|
||
| // 🔴 ВАЖНО: сбрасываем выбранного человека | ||
| onSelected(null); |
There was a problem hiding this comment.
Small issue related to Checklist: Although you reset selected person via onSelected(null) in handleChange, there's no differentiation between raw input changes that are only whitespace. Per requirement, when the selected person is shown, and the input value changes, the selected person should be cleared. But if input change is spaces-only and you decide to ignore spaces for filtering, you still must clear the selected person. Ensure you still call onSelected(null) on every input mutation (including spaces) if the intent is to clear selection; or document the intended behavior and adjust accordingly.
| {selectedPerson | ||
| ? `${selectedPerson.name} (${selectedPerson.born} - ${selectedPerson.died})` | ||
| : 'No selected person'} | ||
| </h1> |
There was a problem hiding this comment.
Checklist item: "don't generate key on render" — in peopleList.tsx the list uses key={person.slug}, which is stable and correct. No action needed here. (No violating pattern found.)
| <> | ||
| {people.map(person => ( | ||
| <div | ||
| key={person.slug} |
There was a problem hiding this comment.
Checklist item: "don't generate key on render" is satisfied (you use stable person.slug), so no issue there. However, ensure slug is unique for all people — otherwise using index or generated keys on render would violate the checklist. (See peopleList key usage at line 16)
DEMO LINK