Skip to content

Solution#1152

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

Solution#1152
Stan0men wants to merge 3 commits intomate-academy:masterfrom
Stan0men:develop

Conversation

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

I am requesting changes because a few core requirements are not fully met yet and they are likely to break the expected behavior and tests.

Critical issues to fix:

  • In src/App.tsx, in the useEffect that debounces and filters people (the one depending on [query, delay, people]), you compute const queryLower = query.trim().toLowerCase(); but still depend on the raw query. This means the effect re-runs and refilters when only whitespace changes (e.g. "John""John "), violating the requirement to avoid rerunning filtering when the trimmed text has not changed; derive the trimmed/lowercased value outside and depend on that, or store the last trimmed value in a ref and skip scheduling filtering when it is unchanged.
  • The requirement to show the full list immediately when the input is focused and empty is not satisfied because suggestions are only populated after the debounce delay when focusing an empty field. In the component that handles focus and filtering (your autocomplete/input component), adjust the onFocus logic so that if the trimmed query is empty you immediately set suggestions to all people and open the dropdown without waiting for the debounce (while still debouncing normal typing).
  • Tests expect data-qa attributes, but your components currently use data-cy (for example on the title, input, and suggestion elements). To avoid test failures, rename or add the expected data-qa attributes on those same elements (e.g. data-qa="title", data-qa="search-input", and the appropriate attributes for suggestion items and the “no suggestions” message).

Aside from these issues, the overall structure of your autocomplete (debounce logic, selection handling, and title rendering) is solid, so once you address the points above your solution should 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/App.tsx
<Input
person={peopleFromServer}
delay={300}
onSelected={person => setSelectedPerson(person)}
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 compute a trimmed/lowercased version of the query here: const queryLower = query.trim().toLowerCase(); — but you still rely on the raw query in the effect dependencies. As a result the effect will re-run when query changes by whitespace only (e.g. "John" -> "John ") even though the trimmed text didn't change, causing redundant filtering. The task requires: "Don't run filtering again if the text has not changed (a pause in typing happened when the text was the same as before)." Consider deriving queryLower outside and using it in the dependency array or storing the last trimmed value in a ref and skipping filtering when it hasn't changed.

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 one remaining issue is likely to break the automated tests, even though the core autocomplete behavior is implemented very well. In src/components/Input.tsx, the suggestions container (the div with className="dropdown-content" that wraps the list of people) is missing the required data-qa="suggestions-list" attribute; the tests depend on this to reliably locate the suggestions list, so this needs to be added to satisfy the checklist and ensure the test suite can pass.

There are also a couple of small improvements that are not blockers but worth addressing: the "No matching suggestions" message currently checks query !== '', which can briefly show the message for inputs that are only spaces, so using query.trim() in that condition would better match the requirement about treating spaces-only input as empty. Additionally, the person prop in Input actually holds an array, so renaming it to people and updating the usage in src/App.tsx would improve clarity, and replacing href="#" on suggestion links with a <button> or preventing default navigation would avoid minor UX quirks.

Once you add the data-qa="suggestions-list" attribute and, optionally, refine the minor points, your solution will fully align with both the functional requirements and the testing expectations—overall the debounce logic, Bulma dropdown usage, selection handling, and state management look solid, so you are very close.


✨ 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

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

Your solution is approved because it cleanly fulfills the autocomplete requirements: you use Bulma’s dropdown correctly, implement a debounced search with a configurable delay, show all people on focus when the input is empty, handle spaces-only input without unnecessary filtering, and correctly propagate the selected person back to the App title and input field.
I’m treating this as an educational override so you can keep progressing while you explore a couple of small refinements.
There are no critical issues to fix, but you could improve readability by extracting inline event handlers (like the ones passed directly to onChange and onFocus in App.tsx and Input.tsx) into named functions such as handleInputChange and handleFocus, which makes the components easier to scan and test.
As a minor UX/accessibility enhancement, consider adding keyboard support for selecting suggestions (e.g., handling Arrow Up/Down and Enter on the input to move through and choose items), since selection currently relies on mouse interaction.
Overall, your implementation is solid, matches the functional requirements closely, and these suggestions are about polishing an already good piece of work rather than fixing mistakes—nice job.


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

Footnotes

  1. Rate AI review example

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