fix(toolbar): prevent event listener memory leak in play button #5100
+8
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replace addEventListener with onclick assignment for stop button handler to prevent duplicate event listeners from accumulating each time the play button is clicked. The previous implementation added a new click listener to the stop button every time the play button was clicked, causing: - Memory leak from accumulating listeners - Multiple handlers firing on single click - Performance degradation over time This fix follows the pattern used elsewhere in toolbar.js (e.g., renderStopIcon) and matches recent memory leak fixes in widgets (sugarlabs#5030, sugarlabs#5029, sugarlabs#4950, sugarlabs#4913). Changed Files: - js/toolbar.js: Replace addEventListener with onclick - js/__tests__/toolbar.test.js: Update test to match new implementation
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a memory leak in the toolbar play button by replacing addEventListener with .onclick assignment. The memory leak occurred because the stop button's event listener was being added inside the play button's click handler, causing duplicate listeners to accumulate with each play button click.
- Changed
stopIcon.addEventListener("click", ...)tostopIcon.onclick = ...in the play button handler - Updated test assertions to verify
onclickassignment instead ofaddEventListenercalls - Added explanatory comment in tests about the memory leak prevention
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| js/toolbar.js | Replaced addEventListener with .onclick assignment to prevent accumulating event listeners on the stop button |
| js/tests/toolbar.test.js | Updated test to verify onclick assignment and added comment explaining the memory leak fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
forgot to turn off copilot, appologies |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace addEventListener with onclick assignment to prevent duplicate event listeners from accumulating each time the play button is clicked.
Problem
The stop button listener was added inside the play button's onclick handler. Since the play button can be clicked repeatedly, addEventListener keeps adding new listeners without removing previous ones, causing:
Solution
Use
.onclickassignment which automatically replaces the previous handler instead of accumulating listeners.Changes
js/toolbar.js: ChangedaddEventListenerto.onclickjs/__tests__/toolbar.test.js: Updated test to match new implementationTesting
Fixes Memory leak in play button from accumulating event listeners #5099