Skip to content

add solution#1153

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

add solution#1153
RuslanV23 wants to merge 3 commits intomate-academy:masterfrom
RuslanV23:develop

Conversation

@RuslanV23
Copy link
Copy Markdown

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 requesting changes because a few critical requirements from the task and checklist are not yet fully met, even though the overall behavior is close.

Critical issues that must be fixed:

  • In src/components/Autocomplete.tsx, the input onChange handler currently calls the debounced applyQuery with the raw value, so a query of only spaces still triggers filtering, which violates the requirement that filtering must not run for spaces-only input [CHECKLIST ITEM #1]; trim the value and skip calling the debounced setter when the trimmed value is empty (treat it as an empty query instead).
  • The component attaches global document listeners (document.addEventListener / document.removeEventListener) to detect outside clicks, which goes against the guideline to avoid direct DOM interaction [CHECKLIST ITEM #2]; prefer a React-driven approach such as relying on onBlur/onFocus on a wrapper or using an existing React outside-click hook instead of touching document directly in this component.
  • The delay prop is required in the Autocomplete props and has no default, but the task explicitly requires a default of 300ms; make delay optional in the type and set delay = 300 in the destructured props so the component behaves correctly even when the parent omits it.
  • The "No matching suggestions" message is rendered whenever filterPeople.length === 0, even when the dropdown is closed; update the rendering condition so it only appears when the menu is open (e.g. isOpenMenu && filterPeople.length === 0) as part of the suggestions UI.

Minor but important improvements:

  • The input change handler is typed as a FocusEvent instead of a ChangeEvent, so update its signature to use React.ChangeEvent<HTMLInputElement> for correctness and clearer intent, and consider trimming the query once upfront before filtering. Overall, you’ve implemented most of the core interactions correctly (debounced suggestions, selection, clearing on change), and once these issues are addressed your solution will align well with 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/components/Autocomplete.tsx Outdated
Comment on lines +39 to +52
const handleQueryChange = (event: React.FocusEvent<HTMLInputElement>) => {
setInputValue(event.target.value);
applyQuery(event.target.value);
onSelected(null);
};

const filterPeople = useMemo(() => {
return people.filter(person => {
return person.name
.trim()
.toLocaleLowerCase()
.includes(appliedQuery.toLocaleLowerCase());
});
}, [appliedQuery, people]);
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 code calls applyQuery with the raw input value and then filters using appliedQuery. This allows filtering to run when the user types only spaces. The checklist requires: "- make sure that filter won't be called if user enred spaces only;" Consider trimming the input and avoiding calling the debounced setter when the trimmed value is empty (or explicitly treat a spaces-only input as an empty query).

Comment thread src/components/Autocomplete.tsx Outdated
Comment on lines +57 to +74
useEffect(() => {
const handleClickOutside = (event: MouseEvent) => {
if (
menuRef.current &&
!menuRef.current.contains(event.target as Node) &&
inputRef.current &&
!inputRef.current.contains(event.target as Node)
) {
setIsOpenMenu(false);
}
};

document.addEventListener('mousedown', handleClickOutside);

return () => {
document.removeEventListener('mousedown', handleClickOutside);
};
}, []);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're attaching a document-level event listener to detect outside clicks. The checklist states: "- don't interact with DOM directly, use React as much as possible;" Adding/removing listeners on document is direct DOM interaction. Prefer using React events (for example, onBlur/onFocus handling on the input/wrapper or managing focus within React) or a React-friendly outside-click hook that still avoids manipulating document directly from component logic.

Comment on lines +25 to +29
export const Autocomplete: React.FC<Props> = ({
people,
delay,
onSelected,
}) => {
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 task requires: "the delay should be customizable via props (default value is 300ms)" but the component's Props mark delay as required and the destructured props don't provide a default. Provide a default value (for example: ({ people, delay = 300, onSelected })) or make delay optional in the props type with a default of 300ms inside the component.

Comment thread src/components/Autocomplete.tsx Outdated
Comment on lines +121 to +135
{filterPeople.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 No matching suggestions message is rendered whenever filterPeople.length === 0, even if the suggestions menu is closed. The requirement says: "show No matching suggestions message if there are no people containing the entered text;" Make the message conditional on the menu being open (e.g., isOpenMenu && filterPeople.length === 0) so the message appears only as part of the open suggestions UI.

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

Changes are needed before this can be considered complete, mainly around the input handling and when filtering is triggered. In src/components/Autocomplete.tsx, the input onChange handler is currently typed as a focus event; this must be changed to React.ChangeEvent<HTMLInputElement> so that event.target.value is correctly typed and the handler matches the required signature. Also in that same handler, you are passing the raw input value directly into the debounced query setter, which means an input of only spaces will still trigger debounced filtering; instead, trim the value, and when the trimmed value is empty, immediately set the applied query to '' and avoid calling the debounced function at all. Additionally, the task requires that filtering is not re-run when the text has not changed: before scheduling the debounced call, compare the (trimmed) new value to the current appliedQuery and skip calling the debounced function when they are equal. These three issues are considered critical because they explicitly violate the task/checklist requirements even though most other behaviors (debounce, selection, showing all people on focus, and “No matching suggestions” visibility) are implemented correctly. Once you fix the event type and adjust the change handler logic to trim input, ignore spaces-only queries, and guard against unchanged queries, your solution will fully align with the specification—you're very close, and these focused tweaks will solidify an otherwise strong implementation.


✨ 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/Autocomplete.tsx Outdated
[setAppliedQuery, delay],
);

const handleQueryChange = (event: React.FocusEvent<HTMLInputElement>) => {
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 input change handler is typed as a focus event. Update it to use the required type: "The input onChange handler must be typed as React.ChangeEvent<HTMLInputElement> (not as a focus event)". This will correctly type event.target.value and match the task requirement.

Comment thread src/components/Autocomplete.tsx Outdated
Comment on lines +40 to +41
setInputValue(event.target.value);
applyQuery(event.target.value);
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 currently calls the debounced setter with the raw value, so a query of only spaces will trigger filtering. Fix per the checklist: "- make sure that filter won't be called if user enred spaces only;" — trim the input and, when the trimmed value is empty, treat it as an empty query (e.g. set setAppliedQuery('') synchronously) and do NOT call the debounced setter with spaces-only input.

Comment thread src/components/Autocomplete.tsx Outdated

const handleQueryChange = (event: React.FocusEvent<HTMLInputElement>) => {
setInputValue(event.target.value);
applyQuery(event.target.value);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no guard to avoid re-running filtering when the text hasn't changed. The task states: "- don't run filtering again if the text has not changed (a pause in typing happened when the text was the same as before)". Before calling the debounced setter, compare the new value (or its trimmed form) to the current appliedQuery and skip calling applyQuery if they are equal.

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 your solution as an educational override so you can keep progressing, even though there are still a couple of behaviors to refine. You did a nice job implementing the Bulma dropdown structure, debounced searching, selection handling, and correctly updating the h1 title from the selected person. The main remaining issue is in src/components/Autocomplete.tsx, in the handleQueryChange logic used by the input’s onChange (around the middle of the file): when the user types only spaces, you still call the debounced query setter with the raw newValue, which violates the requirement that the filter must not run for a spaces-only query [CHECKLIST ITEM #1]; instead, trim the value, and if the trimmed value is empty, immediately call setAppliedQuery('') and return without scheduling the debounce. In that same function, the debounced setter should receive the trimmed/normalized text rather than the raw input so appliedQuery never stores just whitespace. Also in handleQueryChange, you currently clear the selected person (onSelected(null)) on every keystroke; to match the requirement more precisely, only clear the selection after you’ve compared the trimmed new value to the current appliedQuery and confirmed that the effective query has actually changed, so brief pauses or unchanged text don’t wipe the selection unexpectedly. Overall, your structure and adherence to most of the checklist is strong, and tightening up these small edge cases will make your Autocomplete feel much more robust and polished.


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

Footnotes

  1. Rate AI review example

Comment on lines +42 to +46
setInputValue(newValue);
onSelected(null);

if (normalizedNewValue === trimAppliedQuery) {
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're clearing the selected person unconditionally here. According to the requirements, the selected person should be cleared when the input value actually changes (so the title updates to 'No selected person'). Move the onSelected(null) call so it only runs when the trimmed/normalized new value differs from the current applied query (i.e., after the equality check), otherwise a pause/unchanged text will still clear the selection unexpectedly.

Comment on lines +39 to +49
const newValue = event.target.value;
const normalizedNewValue = newValue.trim().toLowerCase();

setInputValue(newValue);
onSelected(null);

if (normalizedNewValue === trimAppliedQuery) {
return;
}

applyQuery(newValue);
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 code currently calls the debounced query setter with the raw newValue and will schedule filtering when the user types spaces-only. This violates the checklist item: "make sure that filter won't be called if user enred spaces only;". Instead, when normalizedNewValue is empty (spaces or empty string) you should immediately set appliedQuery to '' (not via debounce) and return. Also, when scheduling the debounced setter, pass the trimmed/normalized value (not the raw newValue) so appliedQuery stores the normalized text rather than whitespace.

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