fix(audio): survive navigation, clear stale state, silence noisy logs
Bug 1 — playback stops on navigation: WaveSurfer v7 creates its <audio> element inside the container div. When SongPage unmounts, the container is removed from the DOM, taking the audio element with it and stopping playback. Fix: AudioService owns a persistent hidden <audio> element on document.body and passes it to WaveSurfer via the `media` option. WaveSurfer uses it for playback but does not destroy it on WaveSurfer.destroy(), so audio survives any number of navigations. Bug 2 — stale playhead/duration when opening a new song: initialize() called destroyWaveSurfer() but never reset the store, so the previous song's currentTime, duration, and isPlaying leaked into the new song's load sequence. Fix: reset those three fields in the store immediately after tearing down the old WaveSurfer instance. cleanup() also now resets duration. Bug 3 — excessive console noise on mobile: Remove console.warn from play() (silent return when not ready) and from useWaveform's play() wrapper. Only console.error on actual errors remains. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -80,12 +80,8 @@ export function useWaveform(
|
||||
}, [options.url, options.songId, options.bandId]);
|
||||
|
||||
const play = () => {
|
||||
if (!audioService.isWaveformReady()) {
|
||||
console.warn('[useWaveform] play() called but not ready', { url: options.url });
|
||||
return;
|
||||
}
|
||||
audioService.play(options.songId ?? null, options.bandId ?? null)
|
||||
.catch(err => console.error('useWaveform.play failed:', err));
|
||||
.catch(err => console.error('[useWaveform] play failed:', err));
|
||||
};
|
||||
|
||||
const pause = () => {
|
||||
|
||||
@@ -7,6 +7,11 @@ class AudioService {
|
||||
private currentUrl: string | null = null;
|
||||
private isReady = false;
|
||||
private lastTimeUpdate = 0;
|
||||
// Persistent audio element attached to document.body so playback survives
|
||||
// SongPage unmounts. WaveSurfer v7 supports passing an existing media element
|
||||
// via the `media` option — it uses it for playback but does NOT destroy it
|
||||
// when WaveSurfer.destroy() is called.
|
||||
private mediaElement: HTMLAudioElement | null = null;
|
||||
|
||||
private constructor() {}
|
||||
|
||||
@@ -19,9 +24,21 @@ class AudioService {
|
||||
|
||||
// For use in tests only
|
||||
public static resetInstance(): void {
|
||||
if (this.instance?.mediaElement) {
|
||||
this.instance.mediaElement.remove();
|
||||
}
|
||||
this.instance = undefined as any;
|
||||
}
|
||||
|
||||
private getOrCreateMediaElement(): HTMLAudioElement {
|
||||
if (!this.mediaElement) {
|
||||
this.mediaElement = document.createElement('audio');
|
||||
this.mediaElement.style.display = 'none';
|
||||
document.body.appendChild(this.mediaElement);
|
||||
}
|
||||
return this.mediaElement;
|
||||
}
|
||||
|
||||
public async initialize(container: HTMLElement, url: string): Promise<void> {
|
||||
if (!container) throw new Error('Container element is required');
|
||||
if (!url) throw new Error('Valid audio URL is required');
|
||||
@@ -29,11 +46,17 @@ class AudioService {
|
||||
// Reuse the existing instance when the URL hasn't changed
|
||||
if (this.currentUrl === url && this.wavesurfer) return;
|
||||
|
||||
// Tear down the previous instance before creating a new one
|
||||
if (this.wavesurfer) this.destroyWaveSurfer();
|
||||
// Tear down the previous instance and clear stale store state
|
||||
if (this.wavesurfer) {
|
||||
this.destroyWaveSurfer();
|
||||
usePlayerStore.getState().batchUpdate({ isPlaying: false, currentTime: 0, duration: 0 });
|
||||
}
|
||||
|
||||
const ws = WaveSurfer.create({
|
||||
container,
|
||||
// Provide a persistent audio element so playback continues even when
|
||||
// the SongPage container div is removed from the DOM on navigation.
|
||||
media: this.getOrCreateMediaElement(),
|
||||
waveColor: "rgba(255,255,255,0.09)",
|
||||
progressColor: "#c8861a",
|
||||
cursorColor: "#e8a22a",
|
||||
@@ -80,10 +103,7 @@ class AudioService {
|
||||
}
|
||||
|
||||
public async play(songId: string | null = null, bandId: string | null = null): Promise<void> {
|
||||
if (!this.wavesurfer || !this.isReady) {
|
||||
console.warn('[AudioService] play() called before ready');
|
||||
return;
|
||||
}
|
||||
if (!this.wavesurfer || !this.isReady) return;
|
||||
await this.wavesurfer.play();
|
||||
if (songId && bandId) {
|
||||
usePlayerStore.getState().setCurrentSong(songId, bandId);
|
||||
@@ -120,13 +140,14 @@ class AudioService {
|
||||
this.destroyWaveSurfer();
|
||||
const store = usePlayerStore.getState();
|
||||
store.setCurrentSong(null, null);
|
||||
store.batchUpdate({ isPlaying: false, currentTime: 0 });
|
||||
store.batchUpdate({ isPlaying: false, currentTime: 0, duration: 0 });
|
||||
}
|
||||
|
||||
private destroyWaveSurfer(): void {
|
||||
if (!this.wavesurfer) return;
|
||||
try {
|
||||
if (this.wavesurfer.isPlaying()) this.wavesurfer.pause();
|
||||
// Don't pause — if audio is playing and we're switching songs, we want
|
||||
// the pause to happen naturally when the media element src changes.
|
||||
this.wavesurfer.unAll();
|
||||
this.wavesurfer.destroy();
|
||||
} catch (err) {
|
||||
|
||||
@@ -129,6 +129,22 @@ describe('AudioService', () => {
|
||||
expect(vi.mocked(WaveSurfer.create).mock.calls.length).toBe(createCallCount); // no new instance
|
||||
});
|
||||
|
||||
it('resets store state when URL changes', async () => {
|
||||
await initService(service, { url: 'http://example.com/a.mp3', duration: 180 });
|
||||
usePlayerStore.getState().batchUpdate({ isPlaying: true, currentTime: 45 });
|
||||
|
||||
const WaveSurfer = (await import('wavesurfer.js')).default;
|
||||
const mockWs2 = createMockWaveSurfer();
|
||||
vi.mocked(WaveSurfer.create).mockReturnValue(mockWs2 as any);
|
||||
const p2 = service.initialize(makeContainer(), 'http://example.com/b.mp3');
|
||||
mockWs2._emit('ready');
|
||||
await p2;
|
||||
|
||||
// State should be reset, not carry over from the previous song
|
||||
expect(usePlayerStore.getState().currentTime).toBe(0);
|
||||
expect(usePlayerStore.getState().isPlaying).toBe(false);
|
||||
});
|
||||
|
||||
it('destroys old instance when URL changes', async () => {
|
||||
const WaveSurfer = (await import('wavesurfer.js')).default;
|
||||
|
||||
@@ -152,10 +168,8 @@ describe('AudioService', () => {
|
||||
|
||||
describe('play()', () => {
|
||||
it('does nothing if not ready', async () => {
|
||||
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
|
||||
await service.play('song-1', 'band-1');
|
||||
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('before ready'));
|
||||
warnSpy.mockRestore();
|
||||
// play() silently returns when not ready — no throw, no warn
|
||||
await expect(service.play('song-1', 'band-1')).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it('calls wavesurfer.play()', async () => {
|
||||
@@ -225,23 +239,14 @@ describe('AudioService', () => {
|
||||
expect(service.isWaveformReady()).toBe(false);
|
||||
});
|
||||
|
||||
it('resets isPlaying and currentTime in the store', async () => {
|
||||
await initService(service);
|
||||
it('resets isPlaying, currentTime, and duration in the store', async () => {
|
||||
await initService(service, { duration: 180 });
|
||||
usePlayerStore.getState().batchUpdate({ isPlaying: true, currentTime: 30 });
|
||||
service.cleanup();
|
||||
const state = usePlayerStore.getState();
|
||||
expect(state.isPlaying).toBe(false);
|
||||
expect(state.currentTime).toBe(0);
|
||||
});
|
||||
|
||||
it('pauses before destroying if playing', async () => {
|
||||
const callOrder: string[] = [];
|
||||
const mockWs = await initService(service);
|
||||
mockWs.isPlaying.mockReturnValue(true);
|
||||
mockWs.pause.mockImplementation(() => callOrder.push('pause'));
|
||||
mockWs.destroy.mockImplementation(() => callOrder.push('destroy'));
|
||||
service.cleanup();
|
||||
expect(callOrder).toEqual(['pause', 'destroy']);
|
||||
expect(state.duration).toBe(0);
|
||||
});
|
||||
|
||||
it('is safe to call when not initialized', () => {
|
||||
|
||||
Reference in New Issue
Block a user