Solution#1166
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is close to completion, but there are two critical issues that violate explicit checklist requirements: Critical Issue #1 - Filter runs for spaces-only input (checklist item #4): The condition appliedQuery ? people.filter(...) : people checks appliedQuery which is truthy for whitespace-only input like " ". However, normalizedQuery becomes an empty string after .trim(), causing people.filter() to still execute. Since every string contains an empty substring, all people match. You need to check normalizedQuery instead (or normalizedQuery.length > 0) to properly skip filtering when only spaces are entered. Critical Issue #2 - Function naming violation (checklist item #4): The function isAppliedQuery implies it returns a boolean, but it returns undefined and performs side effects. Rename it to something action-oriented like applyQueryIfChanged. Medium Issue - Redundant wrapper: The onChange handler wraps an existing function unnecessarily: // Current (redundant) onChange={e => { handleQueryChange(e); }} // Should be onChange={handleQueryChange} Once you fix the spaces-only input filtering and rename the function, the core requirements will be met.
✨ 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 filteredPeople = appliedQuery | ||
| ? people.filter(person => | ||
| person.name.toLowerCase().includes(normalizedQuery)) |
There was a problem hiding this comment.
This violates checklist item #4: 'make sure that filter won't be called if user entered spaces only'. When appliedQuery is only spaces (e.g., ' '), appliedQuery is truthy so people.filter() still runs. Use normalizedQuery in the condition instead: normalizedQuery ? people.filter(...) : people
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const applyQuery = useCallback(debounce(isAppliedQuery, delay), [delay]); |
There was a problem hiding this comment.
The debounced function should be cancelled on unmount to prevent memory leaks. Add a useEffect cleanup: useEffect(() => { return () => applyQuery.cancel(); }, []);
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation successfully handles all the required functionality, including the spaces-only input filtering using normalizedQuery. The code correctly uses React state, properly implements debouncing, and all features from the task description are working as expected.
However, there are two style improvements to address:
-
Function naming convention violation ([CHECKLIST ITEM #4]): The function
isAppliedQueryin bothsrc/App.tsxandsrc/Autocomplete.tsximplies a boolean return value, but it returnsundefinedand performs side effects. Rename it to an action-oriented name likeapplyQueryIfChangedto match the naming conventions for methods that perform actions rather than return values. -
Redundant onChange wrapper: In
src/App.tsx(lines 68-70), the onChange handleronChange={e => { handleQueryChange(e); }}can be simplified toonChange={handleQueryChange}sincehandleQueryChangealready accepts the event as its first argument.
The functionality is solid and all core requirements are met—these are style convention fixes that will improve code clarity. Once you update the function names and simplify the wrapper, you'll have a complete, well-structured implementation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| setSelectedPerson(person); | ||
| setQuery(person.name); | ||
| }} | ||
| delay={delayValue} | ||
| query={query} | ||
| onQueryChange={value => { | ||
| setQuery(value); | ||
| setSelectedPerson(null); |
There was a problem hiding this comment.
Checklist item #4 violation: Function name isAppliedQuery implies a boolean return value, but it returns undefined (early return) and performs side effects. Rename to an action-oriented name like applyQueryIfChanged per naming conventions for methods that return undefined and perform actions.
|
|
||
| const isAppliedQuery = (value: string) => { | ||
| if (lastAppliedQuery.current === value) { | ||
| return; | ||
| } | ||
|
|
||
| lastAppliedQuery.current = value; | ||
| setAppliedQuery(value); |
There was a problem hiding this comment.
This function name implies it returns a boolean (is prefix), but it returns undefined and performs side effects. Per the naming conventions requirement, functions that perform actions should have action-oriented names like applyQueryIfChanged.
| value={query} | ||
| onChange={e => { | ||
| handleQueryChange(e); |
There was a problem hiding this comment.
Redundant wrapper - can be simplified to onChange={handleQueryChange}
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
All functional requirements are met and the implementation works correctly. The spaces-only input filtering is properly handled, debouncing is implemented, and all core features function as expected. The two remaining issues—the misleading function name isAppliedQuery and the redundant onChange wrapper—are style and naming convention issues rather than functional blockers. While the function name does imply a boolean return when it performs side effects, this doesn't break any functionality. For future reference, consider renaming to something action-oriented like applyQueryIfChanged to better communicate its purpose, and simplify onChange={e => { handleQueryChange(e); }} to onChange={handleQueryChange}.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <Autocomplete | ||
| people={peopleFromServer} | ||
| onSelected={person => { | ||
| setSelectedPerson(person); |
There was a problem hiding this comment.
This violates checklist item #4: 'follow naming conventions for methods'. The function name isAppliedQuery implies it returns a boolean, but it returns undefined with side effects. Rename it to something action-oriented like applyQueryIfChanged.
|
|
||
| const isAppliedQuery = (value: string) => { | ||
| if (lastAppliedQuery.current === value) { | ||
| return; | ||
| } | ||
|
|
||
| lastAppliedQuery.current = value; | ||
| setAppliedQuery(value); |
There was a problem hiding this comment.
This violates checklist item #4 'follow naming conventions for methods'. Function name isAppliedQuery implies it returns a boolean, but it returns undefined and performs side effects. Rename to action-oriented name like applyQueryIfChanged.
| value={query} | ||
| onChange={e => { | ||
| handleQueryChange(e); |
There was a problem hiding this comment.
This violates checklist item #4 - redundant wrapper. Change onChange={e => { handleQueryChange(e); }} to onChange={handleQueryChange}.
2 participants
DEMO LINK