Fix: region-clicked event and seek issue inside region (#3911, #3804) #3945
Conversation
WalkthroughThe pull request modifies 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 (
|
![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 (2)
src/plugins/regions.ts (2)
530-532: Consider adding synchronization for rapid zoom/scroll operations.The current implementation might be susceptible to race conditions during rapid zoom/scroll operations. Consider:
- Using a debounce mechanism for the visibility checks
- Adding a state flag to track pending DOM operations
- Using requestAnimationFrame instead of setTimeout for better synchronization with the browser's render cycle
Example implementation:
+private pendingDOMOperation = false; +private debouncedRenderIfVisible = debounce((container, element) => { if (!this.wavesurfer) return const clientWidth = this.wavesurfer.getWidth() const scrollLeft = this.wavesurfer.getScroll() const scrollWidth = container.clientWidth const duration = this.wavesurfer.getDuration() const start = Math.round((region.start / duration) * scrollWidth) const width = Math.round(((region.end - region.start) / duration) * scrollWidth) || 1 const isVisible = start + width > scrollLeft && start < scrollLeft + clientWidth if (this.pendingDOMOperation) return this.pendingDOMOperation = true requestAnimationFrame(() => { if (isVisible && !element.parentElement) { container.appendChild(element) } else if (!isVisible && element.parentElement) { element.remove() } this.pendingDOMOperation = false }) }, 16); // ~60fps
530-532: Enhance error handling for DOM operations.While the core fix is solid, consider adding error handling to make the code more robust:
- Try-catch blocks around DOM operations
- Logging for debugging browser-specific issues
- Cleanup mechanism for error cases
Example implementation:
-if (isVisible && !element.parentElement) { - container.appendChild(element) -} else if (!isVisible && element.parentElement) { - element.remove() -} +try { + if (isVisible && !element.parentElement) { + container.appendChild(element) + console.debug(`Region ${region.id} appended to DOM`) + } else if (!isVisible && element.parentElement) { + element.remove() + console.debug(`Region ${region.id} removed from DOM`) + } +} catch (error) { + console.error(`Failed to update region ${region.id} DOM state:`, error) + // Attempt cleanup + if (element.parentElement === container) { + try { + element.remove() + } catch (cleanupError) { + console.error('Cleanup failed:', cleanupError) + } + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/plugins/regions.ts(1 hunks)
🔇 Additional comments (1)
src/plugins/regions.ts (1)
530-532: Improved DOM manipulation logic addresses browser-specific issues.
The changes effectively prevent redundant DOM operations by:
- Only appending if the element is visible AND doesn't have a parent
- Only removing if the element is not visible AND has a parent
This should resolve the browser-specific issues with region-clicked events and seeking by ensuring consistent DOM state management.
Let's verify the DOM manipulation logic:
✅ Verification successful
DOM manipulation patterns are consistent across the codebase
The verification shows that while there are multiple instances of appendChild and remove operations throughout the codebase, the specific case in regions.ts is unique because it handles dynamic visibility-based DOM manipulation. Other instances are primarily one-time operations for initialization or cleanup.
The parent element check is specifically needed in regions.ts because:
- It's the only place where elements are repeatedly added/removed based on visibility
- Other DOM operations in the codebase are straightforward mount/unmount scenarios
- No other components use similar virtual append logic that could suffer from the same browser-specific issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar DOM manipulation patterns in the codebase
# that might need the same fix for consistency.
# Search for appendChild/remove calls that might need parent element checks
rg -A 2 "appendChild|remove\(\)" --type ts
# Search for element visibility checks that might need parent element checks
ast-grep --pattern 'if ($visible) {
$element.$method($$$)
}'
Length of output: 4879
Short description
Added parentElement check in virtualAppend method to prevent redundant DOM operations that were breaking click event handling during zoomed playback.
Resolves #3911
Resolves #3804
This PR resolves two related issues with region-clicked events and seeking inside regions in WaveSurfer.js:
Issue region-clicked Event Issue on Regions in WaveSurfer Zoomed State for Chrome and Safari #3911:
The
region-clickedevent was not firing on Chrome and Safari when the zoom slider was used. This behavior was inconsistent across browsers, as Firefox worked as expected.Issue [Regions] Clicking inside region does not seek #3804:
Since version 7.8.2, clicking inside a region no longer seeks the audio position correctly on Chrome and Edge. The expected behavior (as seen in Firefox) is that clicking inside a region seeks the audio to the clicked position.
Implementation details
Modified regions.ts virtualAppend method to check element's DOM state before append/remove operations: