Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this can be considered complete, primarily due to two checklist-related issues that will affect tests and code conventions. In src/components/Autocomplete.tsx, the input and suggestions markup currently use data-cy attributes (e.g. data-cy="search-input", data-cy="suggestions-list", data-cy="suggestion-item", data-cy="no-suggestions-message"), but the task explicitly requires data-qa attributes; this will cause automated tests that look for data-qa="search-input", etc., to fail, so please rename these attributes while keeping the same values. Also in src/components/Autocomplete.tsx, the internal focus handler is declared as onFocus and passed to the input’s onFocus prop; per the required handler naming convention [CHECKLIST ITEM], internal functions should use a handleXxx name (e.g. handleFocus) while onXxx is reserved for props, so rename the function and update its usage. These two points are considered critical because they directly conflict with explicit task requirements and testing expectations. Aside from that, your Autocomplete logic (debounce behavior, showing all people on focus, avoiding re-filtering when the query hasn’t changed, selection behavior, and clearing the selection on input change) looks solid and aligns well with the functional requirements. As a minor, non-blocking improvement, consider using a guaranteed-unique key for suggestion items instead of human.name + human.born to avoid potential collisions. Once you update the data attributes and the handler name, your solution should be in great shape.
✨ 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
| data-cy="search-input" | ||
| value={query} | ||
| onChange={handleQueryChange} | ||
| onFocus={onFocus} | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="dropdown-menu" role="menu" data-cy="suggestions-list"> | ||
| <div className="dropdown-content"> | ||
| {suggestions.map((human: Person) => ( | ||
| <div | ||
| key={human.name + human.born} | ||
| className="dropdown-item" | ||
| data-cy="suggestion-item" | ||
| onMouseDown={() => { | ||
| setQuery(human.name); | ||
| setIsOpen(false); | ||
| onSelected(human); | ||
| }} | ||
| > | ||
| <p className="has-text-link">{human.name}</p> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| </div> | ||
|
|
||
| {query.trim() && suggestions.length === 0 && ( | ||
| <div | ||
| className=" | ||
| notification | ||
| is-danger | ||
| is-light | ||
| mt-3 | ||
| is-align-self-flex-start | ||
| " | ||
| role="alert" | ||
| data-cy="no-suggestions-message" |
There was a problem hiding this comment.
This file uses data-cy attributes (e.g. data-cy="search-input", data-cy="suggestions-list", data-cy="suggestion-item", data-cy="no-suggestions-message") but the task requires the data-qa attributes to be present: "Don't remove the data-qa attributes (they are required for tests)." Update these attributes to use data-qa (for example, data-qa="search-input") so tests relying on the expected attribute names will work.
| const onFocus = () => { | ||
| setIsOpen(true); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className={classNames('dropdown', { 'is-active': isOpen })}> | ||
| <div className="dropdown-trigger"> | ||
| <input | ||
| type="text" | ||
| placeholder="Enter a part of the name" | ||
| className="input" | ||
| data-cy="search-input" | ||
| value={query} | ||
| onChange={handleQueryChange} | ||
| onFocus={onFocus} |
There was a problem hiding this comment.
The internal event handler is named onFocus (defined at this line and used as the input's handler). This violates the checklist item: "- follow [these] naming conventions for methods". According to the recommended convention, internal handlers should be named with a handle prefix (e.g. handleFocus) while onX names are used for props. Rename the internal function (and its usage) to handleFocus to follow the guideline and avoid confusion with prop names.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical test-related issue with the title’s data attribute that can break automated checks.
Critical issue (must fix):
- In
src/App.tsx, the<h1>title currently usesdata-cy="title"instead of the requireddata-qa="title". The task explicitly states thatdata-qaattributes must be preserved and tests are written to look fordata-qa="title", so usingdata-cyhere will cause selectors and tests to fail. To fix this, update that title element to usedata-qa="title"and remove or replace thedata-cyattribute so the selector matches the requirement.
Non-blocking suggestion (optional):
- In
src/components/Autocomplete.tsx, consider using a guaranteed-unique field (for example,person.idif available) as the Reactkeyfor suggestion list items instead of a composite likename + born, to avoid potential key collisions.
Functionally your Autocomplete behavior (debouncing, filtering, selection, and clearing) looks solid and aligned with the requirements; once the title’s data attribute is corrected, your solution should be in good shape for the tests to pass.
✨ 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
| export const Autocomplete: React.FC<Props> = ({ | ||
| people, | ||
| delay = 300, | ||
| onSelected, |
There was a problem hiding this comment.
The project instructions say: "Don't remove the data-qa attributes. It is required for tests." The
title currently uses data-cy="title" instead of data-qa="title". Please replace data-cy with data-qa on the title so tests/selectors depending on data-qa="title" will work.
| <main className="section is-flex is-flex-direction-column"> | ||
| <h1 className="title" data-cy="title"> | ||
| {`${name} (${born} - ${died})`} | ||
| {selectedPerson |
There was a problem hiding this comment.
The app's title uses data-cy="title". Tests and the task description require data-qa attributes. Please replace the attribute with data-qa="title". (Requirement: "Don't remove the data-qa attributes. It is required for tests.")
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational pass so you can continue progressing while you address a couple of remaining issues. You did a strong job implementing the core autocomplete behavior: Bulma dropdown styling, debounced filtering with a configurable delay, showing all people on focus when empty, handling selection correctly, and clearing the selected person when the input changes, all while respecting constraints like no direct DOM access and no random keys [CHECKLIST ITEM #1–4]. The main blocking issue for the automated tests is that Cypress is looking for data-cy="title" and data-cy="search-input", but your components currently expose data-qa attributes instead; in src/App.tsx near the <h1> title (around line 10) and in src/components/Autocomplete.tsx where the main <input> is declared (around lines 25–40), add or switch to the exact data-cy attributes expected by the tests so the selectors match. Another improvement is to avoid potential key collisions by replacing key={human.name + human.born} in the suggestions list (in src/components/Autocomplete.tsx, in the .map over filteredPeople) with a truly unique, stable identifier like human.id if your data provides one. You could also optionally render the suggestions list only when the dropdown is open for a small performance and clarity gain, although this is not required. Overall, your implementation shows good attention to the functional requirements and code quality, and with these small adjustments you’ll have both clean code and fully passing tests.
✨ 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
Anton-Kuchmasov
left a comment
Anton-Kuchmasov
left a comment
There was a problem hiding this comment.
Good job, but your DEMO link is not working. Please re-deploy your App and then re-request review again
3 participants
DEMO LINK