Night Shift: fix stale onEnded closure in audio hook#23
Night Shift: fix stale onEnded closure in audio hook#23
Conversation
The onEnded callback was captured in the useEffect closure but not included in the dependency array. If the caller updated onEnded between renders, the stale version from the initial render would fire instead. Use a ref pattern so the ended handler always calls the latest callback without needing to re-run the effect (which would reload all audio). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA single React hook file receives TypeScript syntax standardization (semicolons, trailing commas) alongside a new 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 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 |
|||||||||||||||||||||||||
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: 3
🤖 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`:
- Around line 116-135: The play function can leave stale state when the
requested audio is missing; after stopping any currentlyPlayingRef, ensure you
clear currentlyPlayingRef.current and call setIsPlaying(false) if
audioMapRef.current.get(lineNumber) is undefined or null before returning;
modify the play callback (function play) to check for the audio entry and
explicitly reset currentlyPlayingRef and isPlaying when no audio is found, while
keeping the existing behavior when audio exists (setting playbackRate, loop,
handling play promise and updating currentlyPlayingRef/setIsPlaying).
- Around line 50-59: The bug is that total is set to snippets.length but
snippets.forEach skips entries with empty audioUrl, so loadedCount never reaches
total and ready stays false; fix by either (a) compute total as the number of
snippets with a non-empty audioUrl before starting (e.g., derive total =
snippets.filter(s => s.audioUrl).length) or (b) when skipping inside
snippets.forEach (the if (!audioUrl) return), increment loadedCount and update
setState accordingly; ensure you update setState({ loaded, total, ready: loaded
=== total }) and reference loadedCount, total, setState, snippets.forEach,
audioUrl and audioMapRef.current.clear to apply the change.
- Around line 24-27: Add a new test file for the useAudioPreloader hook (e.g.,
src/hooks/__tests__/useAudioPreloader.test.tsx) that verifies regression
coverage for the onEnded callback behavior: mount a component that calls
useAudioPreloader with a small list of AudioSnippet items, spy/mock
HTMLAudioElement events or provide a fake audio element to avoid network, assert
that when you update the onEnded prop to a new function it is invoked on audio
end using the latest function instance (referencing onEnded and
useAudioPreloader) and that the hook does not re-run its effect or recreate
audio elements when the callback changes (i.e., audio elements remain the same
across rerenders). Use React Testing Library or similar to rerender, advance
timers or dispatch the 'ended' event to trigger callbacks, and assert no
additional audio element loads occurred; run and confirm bun run test passes
locally before committing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useAudioPreloader.ts
📜 Review details
🧰 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
🔇 Additional comments (1)
src/hooks/useAudioPreloader.ts (1)
39-43: Good stale-closure fix foronEnded.Using
onEndedRef.currentin the ended handler is the right approach here and avoids effect re-runs/reloads while still invoking the latest callback.Also applies to: 91-94
| export function useAudioPreloader( | ||
| snippets: AudioSnippet[], | ||
| onEnded?: (lineNumber: number) => void | ||
| onEnded?: (lineNumber: number) => void, | ||
| ): UseAudioPreloaderReturn { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find test files for useAudioPreloader
fd -i 'useaudiopreloader' --type fRepository: EtanHey/songscript
Length of output: 92
🏁 Script executed:
# Find any test files in the hooks directory
fd '(test|spec)\.(ts|tsx)$' src/hooks/ || fd 'hooks' src/ --type dRepository: EtanHey/songscript
Length of output: 130
🏁 Script executed:
# Look at the hook implementation
cat -n src/hooks/useAudioPreloader.ts | head -50Repository: EtanHey/songscript
Length of output: 1822
🏁 Script executed:
# Search for onEnded references in test files
rg -l 'onEnded' --type ts --type tsx src/Repository: EtanHey/songscript
Length of output: 89
🏁 Script executed:
# Search more comprehensively for any test files related to useAudioPreloader
find . -type f -name "*useAudioPreloader*"Repository: EtanHey/songscript
Length of output: 94
🏁 Script executed:
# Check if there are test files in a different location
find src/ -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.tsx" -o -name "*.spec.tsx" \)Repository: EtanHey/songscript
Length of output: 435
🏁 Script executed:
# Search for onEnded in the full hook implementation to understand the fix better
cat -n src/hooks/useAudioPreloader.tsRepository: EtanHey/songscript
Length of output: 6642
🏁 Script executed:
# Check if there's a test directory at root level
find . -maxdepth 2 -type d -name "*test*" -o -name "*__tests__*"Repository: EtanHey/songscript
Length of output: 88
Add test file for useAudioPreloader hook with regression coverage for the onEnded callback behavior.
The hook is missing a test file entirely, violating the coding guideline requiring test files for all utilities. Additionally, add a focused regression test confirming that the onEnded callback (lines 39–42) correctly uses the latest callback instance when it changes between renders, without triggering effect re-runs that would reload audio elements. Ensure bun run test passes locally before committing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useAudioPreloader.ts` around lines 24 - 27, Add a new test file for
the useAudioPreloader hook (e.g.,
src/hooks/__tests__/useAudioPreloader.test.tsx) that verifies regression
coverage for the onEnded callback behavior: mount a component that calls
useAudioPreloader with a small list of AudioSnippet items, spy/mock
HTMLAudioElement events or provide a fake audio element to avoid network, assert
that when you update the onEnded prop to a new function it is invoked on audio
end using the latest function instance (referencing onEnded and
useAudioPreloader) and that the hook does not re-run its effect or recreate
audio elements when the callback changes (i.e., audio elements remain the same
across rerenders). Use React Testing Library or similar to rerender, advance
timers or dispatch the 'ended' event to trigger callbacks, and assert no
additional audio element loads occurred; run and confirm bun run test passes
locally before committing.
| let loadedCount = 0; | ||
| const total = snippets.length; | ||
|
|
||
| // Reset state for new snippets | ||
| setState({ loaded: 0, total, ready: false }) | ||
| audioMapRef.current.clear() | ||
| setState({ loaded: 0, total, ready: false }); | ||
| audioMapRef.current.clear(); | ||
|
|
||
| snippets.forEach(({ lineNumber, audioUrl }) => { | ||
| if (!audioUrl) return | ||
| if (!audioUrl) return; | ||
|
|
There was a problem hiding this comment.
ready can stay false indefinitely when some snippets have empty audioUrl.
total is based on all snippets, but Line 58 skips empty URLs without incrementing loadedCount. In that case, loaded !== total forever.
Proposed fix
- let loadedCount = 0;
- const total = snippets.length;
+ let loadedCount = 0;
+ const loadableSnippets = snippets.filter(
+ ({ audioUrl }) => Boolean(audioUrl),
+ );
+ const total = loadableSnippets.length;
// Reset state for new snippets
setState({ loaded: 0, total, ready: false });
audioMapRef.current.clear();
- snippets.forEach(({ lineNumber, audioUrl }) => {
- if (!audioUrl) return;
+ if (total === 0) {
+ setState({ loaded: 0, total: 0, ready: true });
+ return;
+ }
+
+ loadableSnippets.forEach(({ lineNumber, audioUrl }) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let loadedCount = 0; | |
| const total = snippets.length; | |
| // Reset state for new snippets | |
| setState({ loaded: 0, total, ready: false }) | |
| audioMapRef.current.clear() | |
| setState({ loaded: 0, total, ready: false }); | |
| audioMapRef.current.clear(); | |
| snippets.forEach(({ lineNumber, audioUrl }) => { | |
| if (!audioUrl) return | |
| if (!audioUrl) return; | |
| let loadedCount = 0; | |
| const loadableSnippets = snippets.filter( | |
| ({ audioUrl }) => Boolean(audioUrl), | |
| ); | |
| const total = loadableSnippets.length; | |
| // Reset state for new snippets | |
| setState({ loaded: 0, total, ready: false }); | |
| audioMapRef.current.clear(); | |
| if (total === 0) { | |
| setState({ loaded: 0, total: 0, ready: true }); | |
| return; | |
| } | |
| loadableSnippets.forEach(({ lineNumber, audioUrl }) => { | |
| if (!audioUrl) return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useAudioPreloader.ts` around lines 50 - 59, The bug is that total
is set to snippets.length but snippets.forEach skips entries with empty
audioUrl, so loadedCount never reaches total and ready stays false; fix by
either (a) compute total as the number of snippets with a non-empty audioUrl
before starting (e.g., derive total = snippets.filter(s => s.audioUrl).length)
or (b) when skipping inside snippets.forEach (the if (!audioUrl) return),
increment loadedCount and update setState accordingly; ensure you update
setState({ loaded, total, ready: loaded === total }) and reference loadedCount,
total, setState, snippets.forEach, audioUrl and audioMapRef.current.clear to
apply the change.
| const play = useCallback((lineNumber: number) => { | ||
| // Stop any currently playing audio | ||
| if (currentlyPlayingRef.current) { | ||
| currentlyPlayingRef.current.pause() | ||
| currentlyPlayingRef.current.currentTime = 0 | ||
| currentlyPlayingRef.current.pause(); | ||
| currentlyPlayingRef.current.currentTime = 0; | ||
| } | ||
|
|
||
| const audio = audioMapRef.current.get(lineNumber) | ||
| const audio = audioMapRef.current.get(lineNumber); | ||
| if (audio) { | ||
| audio.currentTime = 0 | ||
| audio.playbackRate = playbackRateRef.current | ||
| audio.loop = loopRef.current | ||
| audio.currentTime = 0; | ||
| audio.playbackRate = playbackRateRef.current; | ||
| audio.loop = loopRef.current; | ||
| audio.play().catch((err) => { | ||
| console.warn(`Failed to play audio for line ${lineNumber}:`, err) | ||
| setIsPlaying(false) | ||
| }) | ||
| currentlyPlayingRef.current = audio | ||
| setIsPlaying(true) | ||
| console.warn(`Failed to play audio for line ${lineNumber}:`, err); | ||
| setIsPlaying(false); | ||
| }); | ||
| currentlyPlayingRef.current = audio; | ||
| setIsPlaying(true); | ||
| } | ||
| }, []) | ||
| }, []); |
There was a problem hiding this comment.
play() can leave stale playback state when target audio is unavailable.
After stopping current audio, if audioMapRef has no entry for the requested line, currentlyPlayingRef and isPlaying are not reset consistently.
Proposed fix
const play = useCallback((lineNumber: number) => {
// Stop any currently playing audio
if (currentlyPlayingRef.current) {
currentlyPlayingRef.current.pause();
currentlyPlayingRef.current.currentTime = 0;
}
const audio = audioMapRef.current.get(lineNumber);
- if (audio) {
- audio.currentTime = 0;
- audio.playbackRate = playbackRateRef.current;
- audio.loop = loopRef.current;
- audio.play().catch((err) => {
- console.warn(`Failed to play audio for line ${lineNumber}:`, err);
- setIsPlaying(false);
- });
- currentlyPlayingRef.current = audio;
- setIsPlaying(true);
- }
+ if (!audio) {
+ currentlyPlayingRef.current = null;
+ setIsPlaying(false);
+ return;
+ }
+
+ audio.currentTime = 0;
+ audio.playbackRate = playbackRateRef.current;
+ audio.loop = loopRef.current;
+ currentlyPlayingRef.current = audio;
+ setIsPlaying(true);
+ audio.play().catch((err) => {
+ console.warn(`Failed to play audio for line ${lineNumber}:`, err);
+ currentlyPlayingRef.current = null;
+ setIsPlaying(false);
+ });
}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const play = useCallback((lineNumber: number) => { | |
| // Stop any currently playing audio | |
| if (currentlyPlayingRef.current) { | |
| currentlyPlayingRef.current.pause() | |
| currentlyPlayingRef.current.currentTime = 0 | |
| currentlyPlayingRef.current.pause(); | |
| currentlyPlayingRef.current.currentTime = 0; | |
| } | |
| const audio = audioMapRef.current.get(lineNumber) | |
| const audio = audioMapRef.current.get(lineNumber); | |
| if (audio) { | |
| audio.currentTime = 0 | |
| audio.playbackRate = playbackRateRef.current | |
| audio.loop = loopRef.current | |
| audio.currentTime = 0; | |
| audio.playbackRate = playbackRateRef.current; | |
| audio.loop = loopRef.current; | |
| audio.play().catch((err) => { | |
| console.warn(`Failed to play audio for line ${lineNumber}:`, err) | |
| setIsPlaying(false) | |
| }) | |
| currentlyPlayingRef.current = audio | |
| setIsPlaying(true) | |
| console.warn(`Failed to play audio for line ${lineNumber}:`, err); | |
| setIsPlaying(false); | |
| }); | |
| currentlyPlayingRef.current = audio; | |
| setIsPlaying(true); | |
| } | |
| }, []) | |
| }, []); | |
| const play = useCallback((lineNumber: number) => { | |
| // Stop any currently playing audio | |
| if (currentlyPlayingRef.current) { | |
| currentlyPlayingRef.current.pause(); | |
| currentlyPlayingRef.current.currentTime = 0; | |
| } | |
| const audio = audioMapRef.current.get(lineNumber); | |
| if (!audio) { | |
| currentlyPlayingRef.current = null; | |
| setIsPlaying(false); | |
| return; | |
| } | |
| audio.currentTime = 0; | |
| audio.playbackRate = playbackRateRef.current; | |
| audio.loop = loopRef.current; | |
| currentlyPlayingRef.current = audio; | |
| setIsPlaying(true); | |
| audio.play().catch((err) => { | |
| console.warn(`Failed to play audio for line ${lineNumber}:`, err); | |
| currentlyPlayingRef.current = null; | |
| setIsPlaying(false); | |
| }); | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useAudioPreloader.ts` around lines 116 - 135, The play function can
leave stale state when the requested audio is missing; after stopping any
currentlyPlayingRef, ensure you clear currentlyPlayingRef.current and call
setIsPlaying(false) if audioMapRef.current.get(lineNumber) is undefined or null
before returning; modify the play callback (function play) to check for the
audio entry and explicitly reset currentlyPlayingRef and isPlaying when no audio
is found, while keeping the existing behavior when audio exists (setting
playbackRate, loop, handling play promise and updating
currentlyPlayingRef/setIsPlaying).
1 participant
User description
Automated improvement by Golems Night Shift.
fix stale onEnded closure in audio hook
PR Type
Bug fix
Description
Fix stale
onEndedclosure by using ref patternEnsures latest callback fires without reloading audio
Apply consistent code formatting with semicolons
Diagram Walkthrough
File Walkthrough
useAudioPreloader.ts
Fix stale closure and apply code formattingsrc/hooks/useAudioPreloader.ts
onEndedRefto store theonEndedcallback and update it oneach render
onEndedcall withonEndedRef.current?.()inhandleEndedhandler
onEndedfrom effect dependencies to prevent unnecessary audioreloading
throughout
Summary by CodeRabbit