Skip to content

Night Shift: fix event listener leak in audio hook #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EtanHey wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Night Shift: fix event listener leak in audio hook #16

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/hooks/useAudioPreloader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ export function useAudioPreloader(
setState({ loaded: 0, total, ready: false })
audioMapRef.current.clear()
Copy link

@coderabbitai coderabbitai bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unloaded audio elements not fully cleaned up on early unmount.

audioMapRef.current only contains elements that successfully fired canplaythrough/canplay (line 65). If the component unmounts while some elements are still loading, the cleanup loop at line 104 never touches them — their canplaythrough/canplay/error one-shot listeners remain live and will fire post-cleanup, calling setState (line 66–70) and writing into an already-cleared audioMapRef. Their audio.src is also never set to '', so the browser continues the network fetch.

This is pre-existing, but a straightforward fix is to track all created audio objects in the same effect scope:

🔧 Proposed fix 
     audioMapRef.current.clear()
+    const allAudio: Array<{ audio: HTMLAudioElement; lineNumber: number }> = []
     const endedHandlers = new Map<number, () => void>()

     snippets.forEach(({ lineNumber, audioUrl }) => {
       if (!audioUrl) return

       const audio = new Audio()
+      allAudio.push({ audio, lineNumber })
       // ... rest of forEach unchanged
     })

     return () => {
-      audioMapRef.current.forEach((audio, lineNumber) => {
+      allAudio.forEach(({ audio, lineNumber }) => {
         audio.pause()
         const handler = endedHandlers.get(lineNumber)
         if (handler) audio.removeEventListener('ended', handler)
+        audio.removeEventListener('canplaythrough', /* handleCanPlay ref needed — see below */)
+        audio.removeEventListener('canplay', /* handleCanPlay ref needed */)
+        audio.removeEventListener('error', /* handleError ref needed */)
         audio.src = ''
       })
       endedHandlers.clear()
       audioMapRef.current.clear()
       currentlyPlayingRef.current = null
     }

handleCanPlay and handleError are already declared with const inside the forEach; storing them in similar per-lineNumber maps (alongside endedHandlers) allows the cleanup to remove them too.

Also applies to: 102-113

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useAudioPreloader.ts` at line 50, The cleanup misses audio elements
that never fired canplay/canplaythrough because only successfully-ready audios
are kept in audioMapRef; to fix, in useAudioPreloader track every created
HTMLAudioElement and its per-audio handlers (handleCanPlay, handleError and
endedHandlers) in a local map alongside audioMapRef, then in the effect cleanup
iterate that full map to removeEventListener for those handlers, pause and set
audio.src = '' (and optionally load()), and clear both the created-audio map and
audioMapRef; ensure you reference the existing symbols audioMapRef,
handleCanPlay, handleError, endedHandlers and perform listener removal for those
same handler functions so no callbacks fire after unmount.


const endedHandlers = new Map<number, () => void>()

snippets.forEach(({ lineNumber, audioUrl }) => {
if (!audioUrl) return

Expand Down Expand Up @@ -92,17 +94,20 @@ export function useAudioPreloader(
audio.addEventListener('canplay', handleCanPlay, { once: true }) // Fallback for cached audio
audio.addEventListener('error', handleError, { once: true })
audio.addEventListener('ended', handleEnded)
endedHandlers.set(lineNumber, handleEnded)
audio.src = audioUrl
audio.load() // Explicitly trigger loading
})

return () => {
// Cleanup all audio elements on unmount
audioMapRef.current.forEach(audio => {
audioMapRef.current.forEach((audio, lineNumber) => {
audio.pause()
audio.removeEventListener('ended', () => {})
const handler = endedHandlers.get(lineNumber)
if (handler) audio.removeEventListener('ended', handler)
audio.src = ''
})
endedHandlers.clear()
audioMapRef.current.clear()
currentlyPlayingRef.current = null
}
Expand Down