Night Shift: fix event listener leak in audio hook#16
Conversation
The cleanup function used `removeEventListener('ended', () => {})` which
is a no-op — the anonymous function doesn't match the original handler
reference. Track ended handlers in a Map so they can be properly removed
during effect cleanup, preventing memory leaks from retained closures.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||
📝 WalkthroughWalkthroughImproves event listener cleanup in the audio preloader hook by introducing an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useAudioPreloader.ts (2)
86-91:⚠️ Potential issue | 🟠 Major
onEndedstale closure: wrong callback silently invoked after prop change.
handleEndedcapturesonEndedfrom the render-time closure (line 88), butonEndedis absent from theuseEffectdependency array (line 114). If the parent re-renders with a newonEndedwhilesnippetsstays referentially identical, everyendedevent will invoke the old callback—silently, with no error.Adding
onEndedto the dep array is not the right fix either; it would tear down and recreate allAudioobjects on every callback identity change.The idiomatic React 19.2 fix is
useEffectEvent, which is imported directly fromreact. Effect Events always "see" the latest values from render (like props and state) without re-synchronizing the Effect, so they are excluded from Effect dependencies.🛡️ Proposed fix using
useEffectEvent-import { useEffect, useState, useRef, useCallback } from 'react' +import { useEffect, useState, useRef, useCallback, useEffectEvent } from 'react'Then, inside the hook body (before the
useEffect), add:+ const handleEndedEvent = useEffectEvent((lineNumber: number) => { + setIsPlaying(false) + onEnded?.(lineNumber) + })And simplify
handleEndedinside thesnippets.forEachcallback:- const handleEnded = () => { - setIsPlaying(false) - if (onEnded) { - onEnded(lineNumber) - } - } + const handleEnded = () => handleEndedEvent(lineNumber)
handleEndedEventalways reads the currentonEndedat call time, so the dep array stays[snippets].Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useAudioPreloader.ts` around lines 86 - 91, The handleEnded function inside useAudioPreloader captures a stale onEnded prop because it's not updated without tearing down Audio objects; replace the closure with an Effect Event: import useEffectEvent from react and create a handler like handleEndedEvent = useEffectEvent((ln) => { setIsPlaying(false); if (onEnded) onEnded(ln); }); Then, inside the snippets.forEach where you currently assign handleEnded to audio.onended, use handleEndedEvent(lineNumber) (or pass the event wrapper) so the audio event always calls the latest onEnded while keeping the useEffect dependency array as [snippets].
1-1:⚠️ Potential issue | 🟠 MajorAdd test file for
useAudioPreloader.ts.The coding guidelines require all helpers and utilities in
**/*.tsto have corresponding test files. This hook implements non-trivial logic (Map-based handler tracking, cleanup ordering, load-state gating) but has no test file, while other hooks in the same directory (e.g.,useAnonymousProgress.ts,useProgress.ts) all have tests following the naming convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useAudioPreloader.ts` at line 1, Add a test file for the hook useAudioPreloader.ts (e.g., useAudioPreloader.test.ts) that verifies its main behaviors: Map-based handler tracking, cleanup/removal ordering, and load-state gating. Use a hook test utility (renderHook from `@testing-library/react-hooks` or React Testing Library) and mock HTMLAudioElement events to simulate load and error; assert that registered handlers stored in the Map are invoked on load, removed on cleanup, and that subsequent calls respect the loaded gate so handlers aren’t re-invoked unnecessarily; also test that removing a specific handler does not affect others. Reference the hook by name useAudioPreloader in your tests and follow the existing test patterns used for useAnonymousProgress.ts and useProgress.ts.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useAudioPreloader.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TanStack Start framework with Bun runtime for the application
Tests must pass locally via
bun run testbefore committing code, as Husky pre-commit hooks will block commits with failing tests
Files:
src/hooks/useAudioPreloader.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All new helpers and utilities MUST have corresponding test files
Files:
src/hooks/useAudioPreloader.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
src/hooks/useAudioPreloader.ts (1)
52-52: The core fix is correct — LGTM.Storing each
handleEndedreference inendedHandlersand retrieving it bylineNumberduring cleanup guaranteesremoveEventListenerreceives the identical function object, which is the only way the browser will actually deregister the listener. TheendedHandlers.clear()on line 110 is a tidy final step.Also applies to: 97-97, 104-111
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useAudioPreloader.ts`:
- 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.
---
Outside diff comments:
In `@src/hooks/useAudioPreloader.ts`:
- Around line 86-91: The handleEnded function inside useAudioPreloader captures
a stale onEnded prop because it's not updated without tearing down Audio
objects; replace the closure with an Effect Event: import useEffectEvent from
react and create a handler like handleEndedEvent = useEffectEvent((ln) => {
setIsPlaying(false); if (onEnded) onEnded(ln); }); Then, inside the
snippets.forEach where you currently assign handleEnded to audio.onended, use
handleEndedEvent(lineNumber) (or pass the event wrapper) so the audio event
always calls the latest onEnded while keeping the useEffect dependency array as
[snippets].
- Line 1: Add a test file for the hook useAudioPreloader.ts (e.g.,
useAudioPreloader.test.ts) that verifies its main behaviors: Map-based handler
tracking, cleanup/removal ordering, and load-state gating. Use a hook test
utility (renderHook from `@testing-library/react-hooks` or React Testing Library)
and mock HTMLAudioElement events to simulate load and error; assert that
registered handlers stored in the Map are invoked on load, removed on cleanup,
and that subsequent calls respect the loaded gate so handlers aren’t re-invoked
unnecessarily; also test that removing a specific handler does not affect
others. Reference the hook by name useAudioPreloader in your tests and follow
the existing test patterns used for useAnonymousProgress.ts and useProgress.ts.
| @@ -49,6 +49,8 @@ export function useAudioPreloader( | |||
| setState({ loaded: 0, total, ready: false }) | |||
| audioMapRef.current.clear() | |||
There was a problem hiding this comment.
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.
1 participant
User description
Automated improvement by GolemsZikaron Night Shift.
fix event listener leak in audio hook
PR Type
Bug fix
Description
Fix event listener memory leak in
useAudioPreloaderhookTrack ended handlers in Map for proper cleanup
Replace ineffective anonymous function removal with actual handler reference
Prevent retained closures from accumulating in memory
File Walkthrough
useAudioPreloader.ts
Track and properly remove audio ended event listenerssrc/hooks/useAudioPreloader.ts
endedHandlersMap to track audio ended event handlers by linenumber
anonymous function
Note
Low Risk
Small, localized change to hook cleanup logic; behavior should be unchanged aside from correctly detaching event listeners.
Overview
Fixes an event-listener leak in
useAudioPreloaderby tracking per-snippetendedhandlers and removing the exact handler functions during effect cleanup.Cleanup now iterates the audio map with
lineNumber, detaches the storedendedlistener, clears the handler map, and then clears audio references.Written by Cursor Bugbot for commit 3fc8807. This will update automatically on new commits. Configure here.
Summary by CodeRabbit