Skip to content

add task solution#1150

Open
freelinex wants to merge 3 commits intomate-academy:masterfrom
freelinex:develop
Open

add task solution#1150
freelinex wants to merge 3 commits intomate-academy:masterfrom
freelinex:develop

Conversation

@freelinex
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Great job on implementing the core functionality of the autocomplete component. The debouncing and state management are well done. However, I am requesting a couple of changes to fully meet the requirements.

First, the filtering logic currently runs when the input consists only of spaces, which violates a checklist requirement.

  • In src/App.tsx, please trim the query before filtering the people data.
  • In src/components/Dropdown.tsx, the debounced onChange handler should prevent calling setQuery if the input value contains only whitespace.

Second, there's a small UI bug: the "No matching suggestions" message remains visible after the dropdown is closed. This message should only be shown when the dropdown is active (isFocused) and there are no results.

Once these points are addressed, your solution will be perfect. Keep up the great work!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +19 to +22
debounce((newValue: string) => {
setQuery(newValue);
onPersonSelect(null);
}, delay),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This debounce callback calls setQuery even when the newValue consists only of spaces. This violates the checklist item: "make sure that filter won't be called if user enred spaces only". Calling setQuery triggers a state update and causes the filter logic in the parent component to run. You should add a condition to prevent this when the input contains only whitespace, while still allowing an empty string to reset the search.

Comment thread src/components/Dropdown.tsx Outdated
Comment on lines +69 to +83
{people.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>
)}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The visibility of the "No matching suggestions" message depends only on the length of the people array. This causes the message to remain visible even after the dropdown is closed (e.g., when the input loses focus). The message should only appear when the user is actively searching and there are no results. Consider adding checks for isFocused and if the input value is not empty to this condition.

Comment thread src/App.tsx Outdated
const [selectedPerson, setSelectedPerson] = useState<Person | null>(null);

const filteredPeople = useMemo(() => {
const searchTerm = query.toLowerCase() || '';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current filtering logic doesn't handle queries that consist only of spaces. According to the checklist, the filter should treat such input as an empty search. Using query.trim() before calling toLowerCase() would solve this by removing leading/trailing whitespace from the search term.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Thanks for your work on this. This requires a few changes before it can be approved.

There are two main issues remaining from the last review:

  1. In Dropdown.tsx, the onChange handler trims the input value as the user types. This prevents typing spaces, making it impossible to search for multi-word names. The input should reflect exactly what the user types, so please use setValue(e.target.value). The trimming should only happen for the debounced query used for filtering.
  2. The "No matching suggestions" message remains visible after the input loses focus. This message should only be shown when the dropdown is active and a search yields no results. Please adjust the visibility condition to depend on the focus state, for example: isFocused && value && people.length === 0.

Additionally, to ensure whitespace-only searches are handled correctly, please trim the query in App.tsx before filtering: const searchTerm = query.trim().toLowerCase();.

You're very close to a complete solution. Please make these adjustments, and we'll be good to go!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/components/Dropdown.tsx Outdated
data-cy="search-input"
value={value}
onChange={e => {
setValue(e.target.value.trim());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line trims the user's input as they type, which can lead to a confusing user experience. For instance, if the user tries to type leading spaces, they will be removed immediately. It's better to display the user's input as-is by using setValue(e.target.value). The trimming you're doing for debouncedSetQuery is correct for the filtering logic.

Comment thread src/components/Dropdown.tsx Outdated
</div>
</div>

{people.length === 0 && (
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition for displaying the 'No matching suggestions' message doesn't fully address the feedback from the previous review. The message will remain visible even after the user clicks away from the input (i.e., when the dropdown is no longer active). The visibility should depend on the focus state. A better condition would be something like isFocused && value && people.length === 0 to ensure it only shows when the dropdown is active and a search has been performed.

Comment thread src/App.tsx Outdated
const [selectedPerson, setSelectedPerson] = useState<Person | null>(null);

const filteredPeople = useMemo(() => {
const searchTerm = query.toLowerCase() || '';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To properly handle queries that contain only whitespace, it's best to trim the query before using it for filtering. This was mentioned in the previous review. Changing this to const searchTerm = query.trim().toLowerCase(); will ensure that inputs with only spaces are treated like an empty search, showing all results.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving this submission as an educational override to support your learning journey—this allows you to move forward while addressing key concepts incrementally. You've done excellent work implementing the debounce with a customizable delay, correctly handling focus-dependent UI states (like the 'No matching suggestions' message), and using Bulma-compliant structure with proper data-cy attributes and stable keys (person.slug). However, there's an important logic bug in Dropdown.tsx: the onMouseDown handler for person selection doesn’t cancel the pending debounced query update, which can cause a race condition where onPersonSelect(null) clears the user’s choice after they’ve clicked. Additionally, resetting query to an empty string on selection leads to inconsistent behavior on re-focus — instead, set query to the selected person’s name. These are valuable learning opportunities around async state management and UX consistency. Keep up the great work — your attention to detail and progress on prior feedback is really evident!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/App.tsx
Comment on lines +27 to +32
<Dropdown
people={filteredPeople}
onPersonSelect={setSelectedPerson}
delay={300}
setQuery={setQuery}
/>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the props passed here are correct, there's a logic issue inside the Dropdown component that affects how they are used, leading to a bug.

  1. Race Condition Bug: The onMouseDown handler for selecting a person in Dropdown.tsx doesn't cancel pending debounced updates. If a user types and quickly selects an item, the delayed update can fire after selection, calling onPersonSelect(null) and clearing their choice. The lodash.debounce function returns an object with a .cancel() method that should be called when an item is selected to prevent this.

  2. UX Issue: After selection, the query is reset to an empty string inside the Dropdown component. This causes the suggestion list to show all items on re-focus, which is inconsistent with the value in the input. The query should be updated to match the selected person's name.

Please fix this logic within the Dropdown component's selection handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants