solve#1365
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This requires a change to meet the project's requirements. Well done on setting up the promise logic correctly! The main issue is that firstPromise rejects with an Error object instead of the required string.
The task specifies that it should reject with the message 'First promise was rejected'. Please update your code to reject with the string directly, like reject('First promise was rejected').
Additionally, as a best practice, you should clean up your event listeners and timers using removeEventListener() and clearTimeout() once a promise is settled. This prevents memory leaks and is a great habit to build for future projects.
✨ 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', (e) => { | ||
| if (e.button === 0) { | ||
| resolve('First promise was resolved'); | ||
| } | ||
| }); | ||
|
|
||
| setTimeout(() => { | ||
| reject(new Error('First promise was rejected')); | ||
| }, 3000); | ||
| }); |
There was a problem hiding this comment.
Once this promise is settled (either resolved on click or rejected by the timeout), the click event listener and the setTimeout timer are not cleaned up. This can lead to memory leaks. Consider storing the timer ID and the event handler function in variables so you can use clearTimeout() and document.removeEventListener() when the promise is done.
| }); | ||
|
|
||
| setTimeout(() => { | ||
| reject(new Error('First promise was rejected')); |
There was a problem hiding this comment.
The requirement asks to reject with the message 'First promise was rejected'. By using new Error(), you're rejecting with an Error object instead of the specified string. It's better to stick to the requirement: reject('First promise was rejected').
| const secondPromise = new Promise((resolve) => { | ||
| document.addEventListener('click', () => { | ||
| resolve('Second promise was resolved'); | ||
| }); | ||
|
|
||
| document.addEventListener('contextmenu', () => { | ||
| resolve('Second promise was resolved'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This promise works, but there's a small improvement to be made. After the promise is resolved by either a left or right click, the event listeners for click and contextmenu remain active. It's a best practice to remove event listeners when they are no longer needed to prevent memory leaks. You could define the event handlers as named functions and then remove them right after calling resolve.
| const thirdPromise = new Promise((resolve) => { | ||
| let leftClicked = false; | ||
| let rightClicked = false; | ||
|
|
||
| document.addEventListener('click', () => { | ||
| leftClicked = true; | ||
|
|
||
| if (rightClicked) { | ||
| resolve('Third promise was resolved'); | ||
| } | ||
| }); | ||
|
|
||
| document.addEventListener('contextmenu', () => { | ||
| rightClicked = true; | ||
|
|
||
| if (leftClicked) { | ||
| resolve('Third promise was resolved'); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Similar to the other promises, the event listeners for click and contextmenu are not removed after the promise is resolved. Once both clicks have occurred and resolve() is called, these listeners are no longer necessary and should be cleaned up using removeEventListener to prevent memory leaks.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good progress on the task, and the logic for the second and third promises is looking great! However, I'm requesting one change that was also mentioned in the last review. The firstPromise needs to reject with the raw string 'First promise was rejected', but it's currently rejecting with an Error object. This is a specific requirement for the task and is causing the tests to fail.
Please update reject(new Error('First promise was rejected')) to reject('First promise was rejected') in src/scripts/main.js. After that small fix, you should be all set!
✨ 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 timerId = setTimeout(() => { | ||
| document.removeEventListener('click', handleClick); | ||
| reject(new Error('First promise was rejected')); |
There was a problem hiding this comment.
This was pointed out in the last review. The requirement is to reject with the string message 'First promise was rejected', not an Error object. Please change this to reject('First promise was rejected') to match the task description.
There was a problem hiding this comment.
Expected the Promise rejection reason to be an Error.eslintprefer-promise-reject-errors
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
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 done a fantastic job implementing all the promises and their corresponding success and error handlers. I'm particularly impressed with how you managed the event listeners, ensuring they were cleaned up correctly after the promises settled. The logic for resolving and rejecting each promise perfectly matches the requirements. This code is clean, efficient, and fully functional. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.