Skip to content

Conversation

@katspaugh
Copy link
Owner

Description

This PR fixes issue #4243 where regions were not properly garbage collected after calling region.remove().

Implementation Details

Root Cause

When region.remove() was called, the region object remained in memory because:

  1. DOM event listeners were not being removed
  2. Content event listeners (for editable content) were not cleaned up
  3. EventEmitter listeners were not cleared
  4. Reactive event streams were not disposed

Solution

  1. Refactored to reactive event streams - Replaced manual addEventListener calls with the codebase's existing fromEvent pattern for better consistency and automatic cleanup
  2. Enhanced remove() method - Added comprehensive cleanup for:
    • All subscriptions (drag streams, event listeners)
    • Content event listeners for editable regions
    • DOM elements
    • EventEmitter listeners via unAll()
  3. Added comprehensive tests - Created 4 new memory leak tests to verify proper cleanup

How to Test It

Automated Tests

yarn jest memory-leaks  # Run the new memory leak tests
yarn test               # Run full test suite

Manual Testing

  1. Create a wavesurfer instance with regions plugin
  2. Add multiple regions
  3. Remove regions using region.remove()
  4. Take a heap snapshot in Chrome DevTools
  5. Verify removed regions are not retained

Checklist

  • Code follows repository style guidelines
  • Tests added for the new functionality
  • All tests pass (83 Cypress + 17 Jest)
  • Linter passes with no errors
  • No breaking changes
  • Documentation updated (N/A - internal fix)

Fixes #4243

Copilot AI review requested due to automatic review settings December 4, 2025 18:13
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Dec 4, 2025

Deploying wavesurfer-js with  Cloudflare Pages  Cloudflare Pages

Latest commit: c7e132b
Status: ✅  Deploy successful!
Preview URL: https://2cee9568.wavesurfer-js.pages.dev
Branch Preview URL: https://fix-region-memory-leak-4243.wavesurfer-js.pages.dev

View logs

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR successfully fixes a memory leak issue where regions were not properly garbage collected after calling region.remove(). The implementation is well thought out and comprehensive.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/plugins/regions.ts Refactored event listeners to use reactive streams and added comprehensive cleanup in remove() method
src/tests/memory-leaks.test.ts Added matchMedia mock and 4 new test cases to verify proper cleanup of region resources

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@katspaugh katspaugh merged commit 3845182 into main Dec 4, 2025
5 checks passed
@katspaugh katspaugh deleted the fix/region-memory-leak-4243 branch December 4, 2025 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gc doesn't clean removed region.

2 participants