Updated player.ts to not pause audio for external media on destroy() #4012
Conversation
In today's solution the playback audio will pause on calling destroy() for instances using external media. We probably dont need this for the external media's.
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
src/player.tsOops! Something went wrong! :( ESLint: 9.17.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![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
🧹 Nitpick comments (1)
src/player.ts (1)
89-89: Remove unnecessary empty lines.The added empty lines don't improve readability in this context. Consider removing lines 89 and 92 while keeping line 91 for separation of the early return.
protected destroy() { - if (this.isExternalMedia) return - this.media.pause()Also applies to: 91-91, 92-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/player.ts(1 hunks)
🔇 Additional comments (2)
src/player.ts (2)
89-92: LGTM! The early return for external media is well-implemented.The change correctly prevents pausing external media on destroy while maintaining the existing behavior for internal media. This aligns with the PR objectives and supports the use case of shared audio elements across multiple WaveSurfer instances.
Line range hint
89-97: Verify memory management for external media.While the early return prevents pausing, we should verify that skipping the subsequent cleanup operations (
remove(),revokeSrc(), etc.) is safe for external media.Let's analyze the cleanup operations:
✅ Verification successful
Current cleanup handling for external media is correct and safe
The early return for external media is appropriate because:
- The media element is owned externally, so
remove()would be destructiverevokeSrc()only affects blob URLs created by the player itself- External media lifecycle is managed by the consumer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to cleanup methods to understand their purpose # and verify they're not critical for external media # Search for remove() calls echo "Searching for media.remove() usage..." rg -A 5 "media\.remove\(\)" # Search for revokeSrc() calls echo "Searching for revokeSrc() usage..." rg -A 5 "revokeSrc\(\)" # Search for media.load() calls echo "Searching for media.load() usage..." rg -A 5 "media\.load\(\)" # Search for tests related to external media cleanup echo "Searching for external media cleanup tests..." rg -i "test.*external.*media.*cleanup"Length of output: 1671
Script:
#!/bin/bash # Search for isExternalMedia definition and usage echo "Searching for isExternalMedia..." rg -A 5 "isExternalMedia" # Search for external media related code echo "Searching for external media patterns..." rg -A 5 "external.*media|media.*external" # Search for blob URL handling echo "Searching for blob URL handling..." rg -A 5 "blob:|URL\.createObjectURL|URL\.revokeObjectURL"Length of output: 4533
|
It's a potentially breaking change for some use cases so I've released it as a new major version (7.9.0). |
Short description
In today's solution the playback audio will pause on calling destroy() for instances using external media.
We probably dont need this for the external media's.
This is a PR which stems from this discussion
Implementation details
Moved the called pause-function after the early return for external media :
How to test it
Create a audio element thats shared globally between n + 1 WaveSurfer instances, then try playing the audio for one of the instances and destroy one of the wavesurfers. The audio will be paused.
Checklist
Summary by CodeRabbit