Add task solution#1361
Conversation
SofiiaLohai
commented
Feb 28, 2026
SofiiaLohai
commented
- DEMO LINK
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on implementing the core logic for the promises. The code correctly handles the different resolution conditions as required.
However, this requires a few changes because resources like timers and event listeners are not being cleaned up after a promise is settled. This can lead to memory leaks and unexpected behavior.
Specifically:
- For
firstPromise, thesetTimeoutis not cancelled when the promise resolves. This will cause an error when it later tries to reject an already-resolved promise. - For all promises, the event listeners (
click,contextmenu) remain active after resolution. This allows success notifications to be triggered multiple times on subsequent clicks.
Please update the code to clean up these resources. You can use clearTimeout() to cancel the timer and removeEventListener() to detach the event handlers right after a promise is resolved.
✨ 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) => { | ||
| document.addEventListener('click', () => { | ||
| resolve('First promise was resolved'); | ||
| }); | ||
|
|
||
| setTimeout(() => { | ||
| reject(new Error('First promise was rejected')); | ||
| }, 3000); | ||
| }); |
There was a problem hiding this comment.
This promise works as required, but there's a potential improvement. Once the promise is resolved by a click, the setTimeout is not cancelled. It will still fire after 3 seconds and attempt to reject the already resolved promise. It's a good practice to clean up timers when they are no longer needed by using clearTimeout() inside the click handler.
| const secondPromise = new Promise((resolve) => { | ||
| 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.
Good job on handling both click types. However, once the promise is resolved (by either a left or right click), the event listeners for both click and contextmenu remain active. This can lead to unnecessary code execution on subsequent clicks. Consider removing both event listeners once the promise is resolved to ensure they only run once.
| const thirdPromise = new Promise((resolve) => { | ||
| let hasLeftClicked = false; | ||
| let hasRightClicked = false; | ||
|
|
||
| const checkBothClicks = () => { | ||
| if (hasLeftClicked && hasRightClicked) { | ||
| resolve('Third promise was resolved'); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('click', () => { | ||
| hasLeftClicked = true; | ||
| checkBothClicks(); | ||
| }); | ||
|
|
||
| document.addEventListener('contextmenu', (e) => { | ||
| e.preventDefault(); | ||
| hasRightClicked = true; | ||
| checkBothClicks(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The logic to track both clicks is correct. Similar to the other promises, after this promise is resolved, the event listeners remain attached to the document. This means every subsequent click will still trigger the handlers. It would be better to remove the event listeners inside checkBothClicks right after the resolve call to prevent this.
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 iteration! You've successfully addressed all the feedback from the previous review by implementing the cleanup logic for timers and event listeners. Your code is now much more robust and fully meets all the functional requirements of the task. I am approving your submission, great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
