Skip to content

solution#1142

Open
danf4691 wants to merge 5 commits intomate-academy:masterfrom
danf4691:develop
Open

solution#1142
danf4691 wants to merge 5 commits intomate-academy:masterfrom
danf4691:develop

Conversation

@danf4691
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

You've built a solid foundation for the autocomplete component, but I'm requesting changes because several key requirements are not met and there are critical bugs that break the functionality.

Here are the main issues to address:

  1. Broken Debounce Logic: In src/App.tsx, you're calling applyQueryDebounce.flush() inside handleQueryChange. This forces the function to run immediately on every keystroke, which defeats the purpose of debouncing. The suggestions should only appear after the user has paused typing.
  2. Incorrect Selection Behavior: The requirements state that selecting a person should save their name to the input. Your current implementation clears the input field instead.
  3. Missing and Incorrect Features:
    • The debounce delay is hardcoded. It needs to be a customizable prop with a default value of 300ms.
    • When no matches are found, the component incorrectly shows the full list of people instead of the 'No matching suggestions' message.
    • Filtering is triggered even when the input contains only spaces, which violates a checklist requirement.
  4. Failing CI Workflow: The test.yml workflow has an invalid flag (-l) in the npm test command and an incorrect path for the test reports, which will cause the CI build to fail.

Please address these points, and I'll be happy to take another look!


✨ 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 +17 to +19
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's a good practice to use the latest versions of GitHub Actions to benefit from new features, bug fixes, and security updates. Consider updating actions/checkout@v2 to v4 and actions/setup-node@v1 to v4.

with:
node-version: ${{ matrix.node-version }}
- run: npm install
- run: npm test -- -l
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 -l flag is not a standard option for npm test or the Cypress test runner. This might be a typo and could cause the test execution to fail. Please verify if this flag is needed or remove it.

uses: actions/upload-artifact@v4
with:
name: report
path: reports
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 path for the test reports seems to be incorrect. Your cypress.config.ts specifies raw_reports as the output directory (reportDir), but this step is trying to upload from reports. This will likely result in no report being uploaded.

src/App.tsx Outdated
);
}

return preparedSuggestions || 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.

When preparedSuggestions is an empty array (i.e., no matches are found), this expression incorrectly returns the full people array. It should return preparedSuggestions (the empty array) so that the UI can show the 'No matching suggestions' message.

);

const suggestions =
query.trim() || isInputFocused
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 logic causes getSuggestions to be called even when the input contains only spaces, as long as the input is focused. This violates checklist item #1: "make sure that filter won't be called if user entered spaces only". The filtering should not be triggered for whitespace-only input.

src/App.tsx Outdated

const handleSelectPerson = (person: Person) => {
setSelectedPerson(person);
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.

According to the requirements, selecting a person should "save selected suggestion text to the input". This implementation clears the input field instead.

src/App.tsx Outdated
debounce((value: string) => {
setQuery(value);
setSelectedPerson(null);
}, 300),
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 debounce delay is hardcoded here. The task requires this delay to be customizable via props, with a default value of 300ms.

Comment on lines +46 to +50
const handleQueryChange = (event: React.ChangeEvent<HTMLInputElement>) => {
applyQueryDebounce(event.target.value);
setSelectedPerson(null);
applyQueryDebounce.flush();
};
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 event handler has a couple of issues:

  1. Calling applyQueryDebounce.flush() immediately negates the purpose of debouncing. It forces the function to execute on every keystroke, rather than waiting for the user to pause typing.
  2. The call to setSelectedPerson(null) here is redundant, as it is already being called inside the debounced function. All related state updates should happen together inside the debounced callback.

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

Excellent progress on this task! You've fixed all the major issues from the last review, and the component is now very close to meeting all requirements.

I'm requesting one change before approving. The main issue is that the suggestions list doesn't close after a user selects an item, which is a key requirement. In src/App.tsx, the dropdown has a permanent is-active class. This should be controlled by a state variable instead. When a user makes a selection in handleSelectPerson, you should update that state to hide the dropdown.

This fix will also likely resolve the failing Cypress test. You're very close to the finish line!


✨ 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

: `${selectedPerson.name} (${selectedPerson.born} - ${selectedPerson.died})`}
</h1>

<div className="dropdown is-active">
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 dropdown remains active permanently, which prevents it from closing when a user selects an item—a key requirement. The is-active class should be toggled based on a state variable (like isInputFocused). To complete the logic, the handleSelectPerson function will also need to update this state to hide the dropdown after a selection.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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

Excellent work on this component and for addressing the previous feedback! I am approving this submission as an educational override to help you progress, but there are a few important issues to review for your next iteration.

In src/App.tsx, there's a bug where clicking a suggestion causes it to be immediately cleared. This happens because the onBlur event handler calls flush(), creating a race condition. Also, the search query should be trimmed to handle inputs with leading or trailing spaces correctly. Finally, the test failure indicates that the suggestion list doesn't reopen after a selection is made, which points to a state management issue.

Addressing these points will make your component much more robust. You're doing a great job, keep it up!


✨ 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

src/App.tsx Outdated
const { name, born, died } = peopleFromServer[0];
function getSuggestions(people: Person[], query: string) {
let preparedSuggestions;
const trimmedQuery = 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 query should be trimmed before being used for filtering. If a user types a name with leading or trailing spaces (e.g., ' Ada '), the current startsWith check will fail to find a match because .trim() is missing here. Also, the variable name trimmedQuery is a bit misleading, as the value is only lowercased, not trimmed.

src/App.tsx Outdated

const handleInputBlur = () => {
setIsInputFocused(false);
applyQueryDebounce.flush();
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 flush() call introduces a bug. When a user clicks a suggestion, the onBlur event for the input can fire. This immediately triggers the debounced function, which calls setSelectedPerson(null) and can clear the person that was just selected.

Removing this line will resolve the issue. The debounced function will still execute after its normal delay if the user clicks away, which is acceptable behavior.

Copy link
Copy Markdown

@IanLuan IanLuan left a comment

Choose a reason for hiding this comment

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

Great progress! However, there are two issues with your task:

  1. When focusing on the empty input, the list is displayed, but we cannot select any items.
  2. The debounce is not working properly. The search is triggering on every keystroke, but it should be debounced.

@danf4691 danf4691 closed this Mar 31, 2026
@danf4691 danf4691 reopened this Mar 31, 2026
@danf4691
Copy link
Copy Markdown
Author

Closed the pull on mistake, the task is done now

@danf4691 danf4691 requested a review from IanLuan March 31, 2026 19:30
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.

3 participants