Fix: Additional bugs and code quality improvements#4210
Merged
Conversation
The constructor's initial load() call silently swallowed errors with .catch(() => null), making debugging difficult when initial load fails. Issues fixed: - Added console.error to log the error for debugging - Error event is still emitted inside load() as documented - Developers can now see initial load failures in console - No change to error event behavior While errors cannot be caught from a constructor call, this provides visibility into initial load failures for debugging purposes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The RecordPlugin created blob URLs with URL.createObjectURL() but never revoked them, causing memory leaks during multiple recordings. Issues fixed: - Added recordedBlobUrl property to track created blob URLs - Revoke previous blob URL before creating new one - Revoke blob URL in destroy() to free memory - Prevents blob accumulation during multiple recording sessions Each recording now properly cleans up its blob URL before creating a new one, preventing memory leaks from unreleased blob references. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The Renderer's destroy() method disconnected the ResizeObserver but didn't clear the reference, causing minor memory retention. Issues fixed: - Set resizeObserver to null after disconnect() - Ensures reference is cleared for garbage collection - Prevents disconnected observer from remaining in memory While the observer was disconnected, the reference prevented it from being garbage collected. Now properly releases the reference. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The drag handler had a pending setTimeout that wasn't cleared when WaveSurfer was destroyed, potentially causing seekTo() calls on destroyed instances. Issues fixed: - Store unsubscribeDrag separately from subscriptions array - Add cleanup function that clears timeout and unsubscribes - Prevents setTimeout callback from executing after destroy - Avoids potential null reference errors The debounce timeout is now properly cleared when the instance is destroyed, preventing orphaned timeout callbacks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The region element initialization could divide by zero if numberOfChannels was 0, resulting in Infinity for elementHeight. Issues fixed: - Added check for numberOfChannels > 0 before division - Prevents Infinity from being set as CSS height value - Falls back to default 100% height for invalid channel counts - Ensures region rendering doesn't break with edge case data The region now safely handles cases where numberOfChannels is 0 or undefined, preventing invalid CSS values. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The convertColorValues() method returned empty string for empty arrays, which could cause rendering issues. Now returns default color. Issues fixed: - Added explicit check for empty color array (length === 0) - Returns default waveColor '#999' instead of empty string - Ensures waveform always has a valid color value - Prevents potential rendering failures with invalid colors Empty color arrays now fall back to a sensible default rather than an empty string that could break rendering. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The startMic() method created new AudioContext instances without closing previous ones when called multiple times, causing resource exhaustion. Issues fixed: - Added check for existing micStream before starting new one - Call stopMic() to clean up previous AudioContext - Prevents AudioContext accumulation from multiple startMic() calls - Ensures system audio resources are properly released Each startMic() call now properly closes the previous AudioContext before creating a new one, preventing audio resource exhaustion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The setContent() method removed old content elements without removing their event listeners first, causing memory leaks from orphaned listeners on detached DOM nodes. Issues fixed: - Store content event listeners in properties for later removal - Remove listeners from old content before detaching it - Re-add listeners to new content after setting it - Prevents memory leaks from orphaned event listeners Event listeners on region content are now properly cleaned up when content is changed, preventing memory leaks from detached DOM nodes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The watchProgress() recursive read() function had no safeguards against malformed streams that never signal completion, potentially causing infinite recursion. Issues fixed: - Added maxIterations limit (100,000) to prevent infinite loops - Added iteration counter to track read attempts - Logs error and exits if limit is reached - Protects against malformed or never-ending streams While reader.read() should eventually return done: true, this provides a safety net for edge cases with malformed streams or protocol errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Deploying wavesurfer-js with
|
| Latest commit: |
d8addbe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6271d33c.wavesurfer-js.pages.dev |
| Branch Preview URL: | https://fix-additional-bugs-and-impr.wavesurfer-js.pages.dev |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR tightens resource management and resilience across the library, addressing leaks and edge cases while improving error visibility.
- Add cleanup for blob URLs, ResizeObserver, and drag debounce timeouts
- Improve error handling and edge-case guards (constructor load logging, channel bounds, empty color arrays, fetch recursion limit)
- Prevent audio resource accumulation by stopping prior mic streams before starting new ones
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wavesurfer.ts | Logs initial load errors and ensures drag debounce timeout is cleared on destroy. |
| src/renderer.ts | Properly disconnects and clears ResizeObserver and handles empty color arrays. |
| src/plugins/regions.ts | Safeguards channel bounds and manages content event listeners during content updates. |
| src/plugins/record.ts | Revokes old blob URLs and stops previous mic sessions to avoid resource accumulation. |
| src/fetcher.ts | Adds a max-iteration guard to prevent infinite recursion when reading streams. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| const read = async () => { | ||
| // Safety check to prevent infinite recursion | ||
| if (iterations++ > maxIterations) { |
There was a problem hiding this comment.
Off-by-one in the max-iteration guard: with iterations++ > maxIterations the loop allows maxIterations + 1 reads. Use >= with post-increment to stop after exactly maxIterations reads.
Suggested change
| if (iterations++ > maxIterations) { | |
| if (iterations++ >= maxIterations) { |
| const read = async () => { | ||
| // Safety check to prevent infinite recursion | ||
| if (iterations++ > maxIterations) { | ||
| console.error('Fetcher: Maximum iterations reached, stopping read loop') |
There was a problem hiding this comment.
[nitpick] When aborting the read loop early, consider canceling the reader to free underlying resources. For example: await reader.cancel().
Suggested change
| console.error('Fetcher: Maximum iterations reached, stopping read loop') | |
| console.error('Fetcher: Maximum iterations reached, stopping read loop') | |
| await reader.cancel() |
Comment on lines
+7
to
+8
| const maxIterations = 100000 // Safety limit to prevent infinite loops | ||
| let iterations = 0 |
There was a problem hiding this comment.
[nitpick] Magic number for iteration cap. Extract to a named constant (e.g., MAX_STREAM_READ_ITERATIONS) or make it configurable to clarify intent and ease tuning.
Comment on lines
+204
to
+205
| // Log error for debugging while still emitting error event | ||
| console.error('WaveSurfer initial load error:', err) |
There was a problem hiding this comment.
[nitpick] Direct console.error in a library can be noisy for consumers. Consider gating behind a debug flag or relying solely on the emitted 'error' event so integrators control logging.
Suggested change
| // Log error for debugging while still emitting error event | |
| console.error('WaveSurfer initial load error:', err) | |
| // Error is emitted via the 'error' event; consumers can handle logging if desired |
|
|
||
| // Clear debounce timeout on destroy | ||
| this.subscriptions.push(() => { | ||
| clearTimeout(debounce) |
There was a problem hiding this comment.
[nitpick] Guard clearTimeout to avoid passing undefined in some TS/lib combinations and to be explicit: if (debounce) clearTimeout(debounce).
Suggested change
| clearTimeout(debounce) | |
| if (debounce) clearTimeout(debounce) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This PR addresses 9 additional bugs and code quality issues found during code analysis. These fixes improve error handling, resource management, and prevent edge case failures.
Issues Fixed
7. Constructor async error logging (src/wavesurfer.ts)
Severity: LOW
8. RecordPlugin blob URL leak (src/plugins/record.ts)
Severity: MEDIUM
9. ResizeObserver reference leak (src/renderer.ts)
Severity: LOW
10. Debounce timeout leak (src/wavesurfer.ts)
Severity: LOW
11. Region channel bounds check (src/plugins/regions.ts)
Severity: LOW
12. Empty color array handling (src/renderer.ts)
Severity: LOW
13. AudioContext accumulation (src/plugins/record.ts)
Severity: MEDIUM
14. Region content event listeners (src/plugins/regions.ts)
Severity: LOW-MEDIUM
15. Fetcher infinite recursion (src/fetcher.ts)
Severity: LOW
Testing Recommendations
Notes
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com