feat: solved task#1375
Conversation
TarasPidkuimukha
commented
Mar 21, 2026
TarasPidkuimukha
commented
- DEMO LINK
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the core logic for the promises! Your use of Promise.all is excellent. However, one requirement isn't quite met, which needs to be fixed. The resolution message for firstPromise must be exactly First promise was resolved, without the exclamation mark.
Additionally, as a point of improvement, your event listeners are never removed after the promises settle. This can lead to memory leaks. A simple way to handle this is to add the { once: true } option to your addEventListener calls, which automatically cleans them up after they fire once. Please update the promise message to meet the requirement.
✨ 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
| ); | ||
|
|
||
| document.addEventListener('click', () => { | ||
| resolve('First promise was resolved!'); |
There was a problem hiding this comment.
The resolution message for firstPromise doesn't exactly match the requirements. It should be First promise was resolved, without the exclamation mark.
| document.addEventListener('click', () => { | ||
| resolve('First promise was resolved!'); | ||
| clearTimeout(id); | ||
| }); |
There was a problem hiding this comment.
This event listener is never removed. After the promise settles (either by resolving on click or rejecting on timeout), the listener remains active. It's a best practice to clean up listeners to avoid memory leaks and unintended side effects. You can achieve this easily by adding the { once: true } option to addEventListener, which automatically removes the listener after it has executed once.
| document.addEventListener('click', () => { | ||
| resolve('Second promise was resolved'); | ||
| }); | ||
|
|
||
| document.addEventListener('contextmenu', (e) => { | ||
| e.preventDefault(); | ||
| resolve('Second promise was resolved'); | ||
| }); |
There was a problem hiding this comment.
Similar to promise1, these event listeners are not removed after the promise resolves. Since the promise only needs to resolve once, consider cleaning them up after the first click event using the { once: true } option in addEventListener.
| document.addEventListener('click', () => { | ||
| resolve('Third promise was resolved'); | ||
| }); | ||
| }); | ||
|
|
||
| const rightClickPromise = new Promise((resolve, reject) => { | ||
| document.addEventListener('contextmenu', (e) => { | ||
| e.preventDefault(); | ||
| resolve(); | ||
| }); |
There was a problem hiding this comment.
The listeners for leftClickPromise and rightClickPromise also remain active after they've fired. Since Promise.all only requires each promise to resolve once, these listeners should be removed after their respective events occur. The { once: true } option is a great fit here as well.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thanks for the update and for implementing the { once: true } suggestion—that's a great improvement!
However, this requires changes because the main issue from the previous review still needs to be addressed. The firstPromise must resolve with the exact message First promise was resolved as specified in the requirements. Your code currently resolves it with First promise was resolved!, and this extra exclamation mark is causing the tests to fail.
Please correct this string in src/scripts/main.js to move forward. Looking forward to your next submission!
✨ 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
| ); | ||
|
|
||
| document.addEventListener('click', () => { | ||
| resolve('First promise was resolved!'); |
There was a problem hiding this comment.
The resolution message here doesn't match the requirement exactly. According to the task description, it should be First promise was resolved, without the exclamation mark.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! I am approving your solution. You've perfectly implemented all the requirements for the promises, and your logic for handling the click events, timeouts, and combining promises with Promise.all is spot-on. It's great to see you successfully addressed the feedback from the previous review. The code is clean, functional, and meets all specifications. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
