Fix: (Renderer) splitChannels memory leak #3959
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
rjett1
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/renderer.ts (3)
296-297: Remove unnecessary optional chainingSince
unsubscribeOnScrollis initialized as an empty array and can never be undefined, the optional chaining (?.) is unnecessary.- this.unsubscribeOnScroll?.forEach((unsubscribe) => unsubscribe()) + this.unsubscribeOnScroll.forEach((unsubscribe) => unsubscribe())
588-597: LGTM: Proper storage of scroll event handlerThe implementation correctly stores the unsubscribe handler in the array, fixing the memory leak by preventing handler overwrite.
Fix indentation
The indentation of the empty line and the array push operation seems incorrect.
const unsubscribe = this.on('scroll', () => { const { scrollLeft } = this.scrollContainer const canvasIndex = Math.floor((scrollLeft / totalWidth) * numCanvases) clearCanvases() draw(canvasIndex - 1) draw(canvasIndex) draw(canvasIndex + 1) }) - - this.unsubscribeOnScroll.push(unsubscribe) + + this.unsubscribeOnScroll.push(unsubscribe)
683-684: LGTM with minor refinementThe cleanup of scroll handlers before re-rendering is correct and necessary. However, like in the destroy method, the optional chaining is unnecessary since
unsubscribeOnScrollis an array.- this.unsubscribeOnScroll?.forEach((unsubscribe) => unsubscribe()) + this.unsubscribeOnScroll.forEach((unsubscribe) => unsubscribe())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer.ts(4 hunks)
🔇 Additional comments (2)
src/renderer.ts (2)
34-34: LGTM: Property type change addresses the memory leak
The change from optional function to array of functions is correct and addresses the core issue by preventing overwrite of unsubscribe handlers.
34-34: Verify complete cleanup of scroll handlers
The implementation looks correct, but let's verify there are no remaining scroll handler references that could cause memory leaks.
Also applies to: 296-297, 588-597, 683-684
✅ Verification successful
Based on the search results, I can now verify the scroll handler cleanup implementation. The code shows:
- The scroll event listener is properly added in the renderer.ts file with a clear cleanup mechanism through the
unsubscribeOnScrollarray. - All scroll-related event handlers are managed through the subscription system:
- In regions.ts: scroll handlers are added to subscriptions and cleaned up when regions are removed
- In timeline.ts: scroll subscriptions are managed through the subscriptions array
- In minimap.ts: scroll handlers are part of the main subscription system
- The main renderer properly cleans up subscriptions in the destroy method.
Let's verify the cleanup implementation with one final check:
Scroll handler cleanup implementation is correct and complete
The verification confirms proper cleanup of scroll handlers:
- The
unsubscribeOnScrollarray is properly initialized and managed - Cleanup is performed in both
destroy()andreRender()methods - All scroll handlers are properly unsubscribed by calling each cleanup function
- The array is reset after cleanup to prevent memory leaks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any scroll-related event handlers that might not be properly cleaned up
rg -A 5 "scroll.*event.*listener"
rg -A 5 "on.*scroll"
Length of output: 19397
Script:
#!/bin/bash
# Check the cleanup implementations in renderer.ts
ast-grep --pattern 'destroy() {
$$$
}'
# Check for any reRender cleanup
ast-grep --pattern 'reRender() {
$$$
}'
Length of output: 86
Script:
#!/bin/bash
# Let's try a different approach to find the cleanup implementations
rg -A 10 "destroy\(\)" src/renderer.ts
rg -A 10 "reRender\(\)" src/renderer.ts
# Also check for unsubscribe usage
rg "unsubscribeOnScroll" src/renderer.ts
Length of output: 2012
Short description
When using wavesurfer's
splitChannelsoption there is a memory leak that occurs when zooming the waveforms. The code does not properly clean up scroll event handlers which can eventually lead to the page crashing.Implementation details
The original code stores an unsubscribe function for a scroll event handler that gets created when an audio channel is rendered. When multiple channels are rendered the previous unsubscribe functions get overwritten and their references are never properly cleaned up.
To fix the issue I've updated the unsubscribeOnScroll variable to be an array, so that it can store the unsubscribe function for each audio channel that gets rendered.
How to test it
You can observe this behavior on the Zoom Plugin example page if you set the
splitChannelsoption totrue. As you zoom in and out the page will use more and more memory.Screenshots
Before:
After:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes