Fix audio player freezing and improve UX#293
Conversation
Switch WaveSurfer from MediaElement to WebAudio backend to prevent WKWebView deadlocks that were freezing the entire Tauri app during audio playback. Reuse a single WaveSurfer instance across track changes instead of destroying and recreating on every URL change, which was exhausting the browser's AudioContext pool. Other improvements: - spacebar play/pause with capture phase to prevent history item activation - drag-to-seek on waveform with silent scrub to avoid WebAudio popping - slider always mounted to prevent layout shift during track transitions - play button fills icon, accent bg when playing, loop button accent bg when active - fix AudioBars animation getting stuck by keying on mode - remove focus ring on history items - sync slider position on pause and during seek - remove title text from player bar - thicker cursor (3px)
📝 WalkthroughWalkthroughThe changes refactor AudioPlayer to streamline WaveSurfer initialization, consolidate playback logic, and remove native device routing. HistoryTable receives minor styling updates, and useSystemAudioCapture undergoes import reordering and formatting adjustments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/components/AudioPlayer/AudioPlayer.tsx (1)
77-81: Unusual effect dependency pattern with early-return guard.The effect runs when
audioUrlchanges but the guardif (wavesurferRef.current) return;prevents re-execution. This works correctly but is non-idiomatic—typically the dependency array would only include values that should trigger re-runs.A cleaner approach would be to use an empty dependency array
[]for true mount-only behavior (since the guard makes subsequent runs no-ops anyway), or move the initialization logic outside the effect.This is a minor style observation and the current implementation is functionally correct.
Also applies to: 238-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/AudioPlayer/AudioPlayer.tsx` around lines 77 - 81, The effect that initializes WaveSurfer currently depends on audioUrl but early-returns when wavesurferRef.current exists; to make this idiomatic, change the useEffect in AudioPlayer that references useEffect, wavesurferRef, and audioUrl to run only on mount (use an empty dependency array []) and keep the guard so initialization happens once, or alternatively move the WaveSurfer creation logic out of the effect into a mount-only helper invoked from a mount-only useEffect; update the effect(s) around the WaveSurfer initialization (the same pattern also used at the other location noted) so dependencies reflect true intent (mount-only) rather than relying on an early-return.app/src/components/History/HistoryTable.tsx (1)
442-446: Consider accessibility impact of removing focus outline.Adding
outline-noneremoves the focus indicator for keyboard users. While the row has hover/active styling, keyboard-only users won't see a visual cue when navigating with Tab. If the intent is to customize the focus ring rather than remove it entirely, consider usingfocus-visible:ring-2 focus-visible:ring-accentor similar instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/History/HistoryTable.tsx` around lines 442 - 446, The JSX row currently removes the keyboard focus indicator by including 'outline-none' in the className (see the className prop where isPlayable and isVersionsExpanded are used); replace 'outline-none' with an accessible focus style such as 'focus-visible:ring-2 focus-visible:ring-accent focus-visible:outline-none' (or your design system's focus classes) so keyboard users get a visible focus ring while preserving existing hover/active styling for the row. Ensure the change is applied to the same element that uses isPlayable and isVersionsExpanded so tab navigation shows the new focus state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/AudioPlayer/AudioPlayer.tsx`:
- Around line 132-147: The timeupdate handler currently duplicates end-of-track
loop logic and can race with the finish handler; remove the loop handling from
the wavesurfer.on('timeupdate') callback (i.e., stop calling
usePlayerStore.getState().isLooping, wavesurfer.seekTo(0), wavesurfer.play(),
wavesurfer.pause(), and setIsPlaying(false) from inside timeupdate) and only
keep timeupdates for updating current time (setCurrentTime(time) and setting
currentTime = dur when time >= dur); keep all loop/finish behavior in the
existing finish event handler so that looping, seekTo, play, pause and
setIsPlaying are handled solely in the finish handler.
- Around line 183-195: The drag-seek mute logic is no-op because
wavesurfer.getMediaElement() returns an HTMLMediaElement in WaveSurfer v7 (no
getGainNode), so seekGain is always null; either remove the block or instantiate
and pass a WebAudioPlayer to WaveSurfer.create() so you can call
webAudioPlayer.getGainNode() and wire the dragstart/dragend handlers to that
GainNode (cancelScheduledValues / setTargetAtTime on its gain using
gain.context.currentTime) instead of calling getGainNode() on the media element;
update references in AudioPlayer.tsx (the wavesurfer setup and this seek
handling block) accordingly.
---
Nitpick comments:
In `@app/src/components/AudioPlayer/AudioPlayer.tsx`:
- Around line 77-81: The effect that initializes WaveSurfer currently depends on
audioUrl but early-returns when wavesurferRef.current exists; to make this
idiomatic, change the useEffect in AudioPlayer that references useEffect,
wavesurferRef, and audioUrl to run only on mount (use an empty dependency array
[]) and keep the guard so initialization happens once, or alternatively move the
WaveSurfer creation logic out of the effect into a mount-only helper invoked
from a mount-only useEffect; update the effect(s) around the WaveSurfer
initialization (the same pattern also used at the other location noted) so
dependencies reflect true intent (mount-only) rather than relying on an
early-return.
In `@app/src/components/History/HistoryTable.tsx`:
- Around line 442-446: The JSX row currently removes the keyboard focus
indicator by including 'outline-none' in the className (see the className prop
where isPlayable and isVersionsExpanded are used); replace 'outline-none' with
an accessible focus style such as 'focus-visible:ring-2
focus-visible:ring-accent focus-visible:outline-none' (or your design system's
focus classes) so keyboard users get a visible focus ring while preserving
existing hover/active styling for the row. Ensure the change is applied to the
same element that uses isPlayable and isVersionsExpanded so tab navigation shows
the new focus state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b13cfb7-76c6-453a-b2d8-f371ae8393b2
📒 Files selected for processing (3)
app/src/components/AudioPlayer/AudioPlayer.tsxapp/src/components/History/HistoryTable.tsxapp/src/lib/hooks/useSystemAudioCapture.ts
| wavesurfer.on('timeupdate', (time) => { | ||
| const dur = usePlayerStore.getState().duration; | ||
| if (dur > 0 && time >= dur) { | ||
| setCurrentTime(dur); | ||
| const loop = usePlayerStore.getState().isLooping; | ||
| if (loop) { | ||
| wavesurfer.seekTo(0); | ||
| wavesurfer.play().catch((err) => debug.error('Loop play failed:', err)); | ||
| } else { | ||
| wavesurfer.pause(); | ||
| setIsPlaying(false); | ||
| } | ||
| } catch (error) { | ||
| debug.error('Failed to fetch runtime channel data:', error); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| debug.log('Auto-play check:', { | ||
| isTauri: platform.metadata.isTauri, | ||
| currentAudioUrl, | ||
| currentProfileId, | ||
| hasProfileChannels: !!runtimeProfileChannels, | ||
| hasChannels: !!runtimeChannels, | ||
| setCurrentTime(time); | ||
| }); |
There was a problem hiding this comment.
Potential duplicate loop handling with finish event.
The timeupdate handler checks time >= dur and handles looping (lines 137-143), but there's also a finish event handler at lines 196-206 that handles the same looping logic. Both could fire in quick succession for the same track end, potentially causing:
- Double
seekTo(0)+play()calls - Race conditions if one fires mid-execution of the other
Consider consolidating loop handling in one place—preferably in the finish event which is the canonical end-of-track signal.
🔧 Suggested consolidation
Remove the loop handling from timeupdate and rely solely on finish:
wavesurfer.on('timeupdate', (time) => {
const dur = usePlayerStore.getState().duration;
if (dur > 0 && time >= dur) {
setCurrentTime(dur);
- const loop = usePlayerStore.getState().isLooping;
- if (loop) {
- wavesurfer.seekTo(0);
- wavesurfer.play().catch((err) => debug.error('Loop play failed:', err));
- } else {
- wavesurfer.pause();
- setIsPlaying(false);
- }
return;
}
setCurrentTime(time);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/AudioPlayer/AudioPlayer.tsx` around lines 132 - 147, The
timeupdate handler currently duplicates end-of-track loop logic and can race
with the finish handler; remove the loop handling from the
wavesurfer.on('timeupdate') callback (i.e., stop calling
usePlayerStore.getState().isLooping, wavesurfer.seekTo(0), wavesurfer.play(),
wavesurfer.pause(), and setIsPlaying(false) from inside timeupdate) and only
keep timeupdates for updating current time (setCurrentTime(time) and setting
currentTime = dur when time >= dur); keep all loop/finish behavior in the
existing finish event handler so that looping, seekTo, play, pause and
setIsPlaying are handled solely in the finish handler.
| const seekMedia = wavesurfer.getMediaElement() as any; | ||
| const seekGain: GainNode | null = seekMedia?.getGainNode?.() ?? null; | ||
| if (seekGain) { | ||
| const ctx = seekGain.context as AudioContext; | ||
| wavesurfer.on('dragstart', () => { | ||
| seekGain.gain.cancelScheduledValues(ctx.currentTime); | ||
| seekGain.gain.setTargetAtTime(0, ctx.currentTime, 0.002); | ||
| }); | ||
| wavesurfer.on('dragend', () => { | ||
| seekGain.gain.cancelScheduledValues(ctx.currentTime); | ||
| seekGain.gain.setTargetAtTime(1, ctx.currentTime, 0.01); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check WaveSurfer version and look for getGainNode usage in codebase/docs
echo "=== Checking WaveSurfer version ==="
cat package.json 2>/dev/null | jq '.dependencies["wavesurfer.js"] // .devDependencies["wavesurfer.js"]'
echo -e "\n=== Searching for getGainNode or getMediaElement patterns ==="
rg -n "getGainNode|getMediaElement" --type ts --type js -C2Repository: jamiepine/voicebox
Length of output: 1793
🌐 Web query:
WaveSurfer.js WebAudio backend getGainNode API
💡 Result:
-
WaveSurfer.js v7+ (recommended way): create a
WebAudioPlayerand pass it asmedia, then callwebAudioPlayer.getGainNode()to get the underlyingGainNode. [1]import WaveSurfer from 'https://unpkg.com/wavesurfer.js@7/dist/wavesurfer.esm.js' import WebAudioPlayer from 'https://unpkg.com/wavesurfer.js@7/dist/webaudio.js' const webAudioPlayer = new WebAudioPlayer() webAudioPlayer.src = '/audio.wav' const ws = WaveSurfer.create({ container: '#wave', media: webAudioPlayer }) const gainNode = webAudioPlayer.getGainNode() gainNode.gain.value = 0.5
-
WaveSurfer.js v6 (legacy “WebAudio backend”): the backend exposes a public
gainNodeproperty (i.e.,wavesurfer.backend.gainNode). [2]
Sources: [1], [2]
🏁 Script executed:
#!/bin/bash
# Find package lock files to determine WaveSurfer version
echo "=== Looking for package lock files ==="
find . -maxdepth 2 -name "package-lock.json" -o -name "yarn.lock" -o -name "pnpm-lock.yaml" | head -5
# Search in all package.json files
echo -e "\n=== Searching for wavesurfer in all package.json files ==="
fd "package.json" --type f | xargs rg "wavesurfer" 2>/dev/null
# Check the app directory for package.json
echo -e "\n=== Checking app/package.json ==="
cat app/package.json 2>/dev/null | head -50Repository: jamiepine/voicebox
Length of output: 2020
🌐 Web query:
WaveSurfer.js version 7 getMediaElement WebAudioPlayer documentation
💡 Result:
getMediaElement() (WaveSurfer.js v7)
WaveSurfer#getMediaElement() returns the underlying HTMLMediaElement used for playback. (wavesurfer.xyz)
const ws = WaveSurfer.create({ /* ... */ })
const mediaEl = ws.getMediaElement() // HTMLMediaElementWebAudioPlayer (v7) docs / API surface
In v7, WebAudioPlayer is a separate helper that shims an audio-element-like interface using Web Audio (i.e., not an actual HTMLMediaElement, so it’s not what getMediaElement() returns). The maintainer shows importing it from the v7 dist/webaudio.js bundle. (github.com)
Its public methods/properties (from the published TypeScript declarations) include src, play(), pause(), currentTime, duration, volume, muted, setSinkId(), plus WebAudio-specific helpers like getGainNode() and getChannelData(). (app.unpkg.com)
import WebAudioPlayer from 'https://unpkg.com/wavesurfer.js@7/dist/webaudio.js'
const player = new WebAudioPlayer()
player.src = '/audio.wav'
await player.play()
const gainNode = player.getGainNode()Citations:
- 1: https://wavesurfer.xyz/docs/classes/wavesurfer.default
- 2: Web Audio API support? katspaugh/wavesurfer.js#3350
- 3: https://app.unpkg.com/wavesurfer.js%407.7.8/files/dist/webaudio.d.ts
🏁 Script executed:
#!/bin/bash
# Check how WaveSurfer is created in AudioPlayer.tsx
echo "=== WaveSurfer instantiation in AudioPlayer.tsx ==="
rg -n "WaveSurfer.create|new WaveSurfer" app/src/components/AudioPlayer/AudioPlayer.tsx -A 10
# Also check for imports
echo -e "\n=== Imports in AudioPlayer.tsx ==="
head -30 app/src/components/AudioPlayer/AudioPlayer.tsx | grep -E "import|from"Repository: jamiepine/voicebox
Length of output: 1522
Remove non-functional drag-seek muting or implement with correct v7 API.
The code calls wavesurfer.getMediaElement()?.getGainNode?.(), but in WaveSurfer v7 with default setup (no WebAudioPlayer passed to create), getMediaElement() returns an HTMLMediaElement which has no getGainNode() method. The optional chaining causes seekGain to silently become null, so the fade-out/fade-in during drag never executes.
To work correctly in v7, either:
- Create and pass a
WebAudioPlayerinstance toWaveSurfer.create()as the media option, then access itsgetGainNode()directly, or - Remove this code entirely if not essential.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/AudioPlayer/AudioPlayer.tsx` around lines 183 - 195, The
drag-seek mute logic is no-op because wavesurfer.getMediaElement() returns an
HTMLMediaElement in WaveSurfer v7 (no getGainNode), so seekGain is always null;
either remove the block or instantiate and pass a WebAudioPlayer to
WaveSurfer.create() so you can call webAudioPlayer.getGainNode() and wire the
dragstart/dragend handlers to that GainNode (cancelScheduledValues /
setTargetAtTime on its gain using gain.context.currentTime) instead of calling
getGainNode() on the media element; update references in AudioPlayer.tsx (the
wavesurfer setup and this seek handling block) accordingly.
Summary
readyevent to avoid race conditions with the WebAudio decode pipelinePlayer UX
History list
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style