add solution#1362
Conversation
ATkachenkoA
commented
Mar 2, 2026
ATkachenkoA
commented
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the promise logic! The secondPromise and notification handlers are working perfectly. However, a couple of changes are needed before this can be approved.
The main issue is with firstPromise. Using { once: true } causes the event listener to be removed after any mouse click. If a user right-clicks first, the listener is detached, and a subsequent left-click will never resolve the promise, causing it to incorrectly reject. The listener should only be removed once the promise is settled.
Additionally, for thirdPromise, the event listener is never removed after the promise resolves. While this doesn't break the functionality, it's inefficient. It's a good practice to clean up listeners using removeEventListener once they are no longer needed to prevent memory leaks.
✨ 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
| resolve('First promise was resolved'); | ||
| } | ||
| }, | ||
| { once: true }, |
There was a problem hiding this comment.
Using { once: true } here introduces a bug. This option removes the listener after any mousedown event. If the first click is a right-click, the listener is removed, and the promise can no longer be resolved with a subsequent left-click. This will cause the promise to be rejected by the timeout, which doesn't meet the requirement that it should resolve on a left click.
| document.addEventListener('mousedown', (e) => { | ||
| if (e.button === 0) { | ||
| leftClicked = true; | ||
| } | ||
|
|
||
| if (e.button === 2) { | ||
| rightClicked = true; | ||
| } | ||
|
|
||
| if (leftClicked && rightClicked) { | ||
| resolve('Third promise was resolved'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This event listener is never removed, even after the promise has been resolved. This is inefficient as it will continue to run on every mouse click for the lifetime of the page. It's a good practice to clean up event listeners when they are no longer needed. Consider defining the listener as a named function and then removing it with removeEventListener right after calling resolve.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on the updates, you're very close to a complete solution! The logic for handling clicks and cleaning up listeners has improved significantly.
However, there is one important issue remaining in firstPromise that requires changes. When the promise is rejected by the timeout, its mousedown event listener is never removed. This creates a memory leak because the listener remains active even after the promise is finished.
Please ensure the event listener is always cleaned up when the promise settles, both when it resolves successfully and when it is rejected. Once you fix this, your solution will be perfect!
✨ 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
|
|
||
| const firstPromise = new Promise((resolve, reject) => { | ||
| const timerId = setTimeout(() => { | ||
| reject(new Error('First promise was rejected')); |
There was a problem hiding this comment.
When this rejection happens, the firstClickHandler is still listening for mousedown events. You should also call document.removeEventListener('mousedown', firstClickHandler) here to clean it up and prevent a memory leak.
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 submission! You've done a fantastic job addressing the previous feedback and correctly implementing the event listener cleanup for firstPromise. Your code is clean, efficient, and fully meets all the requirements for handling the promises and displaying notifications. This is a great implementation, and I am happy to approve your solution. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
