fix: destroy AudioContext on resume timeout for iOS recovery#16
Conversation
iOS Safari can get stuck in a state where resume() times out but the context stays suspended. Previously we only destroyed the context on specific error messages - now we destroy on ANY resume failure. Changes: - Extract attemptResume() DRY helper for all resume paths - Always destroyContext() on resume failure (not just specific errors) - Add statechange listener (from Howler.js PR #1770) - Reduce code by 66 lines through refactoring - Fix test mock to include addEventListener, createBuffer - Add global canvas mock to suppress jsdom warning 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded@idvorkin-ai-tools has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request refactors AudioContext resume logic into a centralized Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 0
🧹 Nitpick comments (1)
src/services/audioService.test.ts (1)
14-14: Consider exposing a method to emit events for more thorough testing.The private
listenersMap tracks event handlers but there's no mechanism to trigger them in tests. This could be useful for testing thestatechangelistener behavior.class MockAudioContext { state: "suspended" | "running" | "closed" = "suspended"; currentTime = 0; destination = {}; private listeners: Map<string, Function[]> = new Map(); + + // Helper to simulate events in tests + emitEvent(event: string): void { + this.listeners.get(event)?.forEach(handler => handler()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/services/audioService.test.ts(5 hunks)src/services/audioService.ts(9 hunks)src/test/setup.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/audioService.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Global AudioContext and iOS unlock mechanisms should be implemented in
src/services/audioService.ts
Files:
src/services/audioService.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: idvorkin/igor-timer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T00:48:06.238Z
Learning: Refer to tech/ios-audio-workaround.md for iOS Safari audio implementation details including AudioContext states, unlock mechanisms, timeout handling, and known quirks
Learnt from: CR
Repo: idvorkin/igor-timer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T00:48:06.238Z
Learning: Applies to src/services/audioService.ts : Global AudioContext and iOS unlock mechanisms should be implemented in `src/services/audioService.ts`
📚 Learning: 2025-12-15T00:48:06.238Z
Learnt from: CR
Repo: idvorkin/igor-timer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T00:48:06.238Z
Learning: Applies to src/services/audioService.ts : Global AudioContext and iOS unlock mechanisms should be implemented in `src/services/audioService.ts`
Applied to files:
src/services/audioService.tssrc/test/setup.tssrc/services/audioService.test.ts
📚 Learning: 2025-12-15T00:48:06.238Z
Learnt from: CR
Repo: idvorkin/igor-timer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T00:48:06.238Z
Learning: Refer to tech/ios-audio-workaround.md for iOS Safari audio implementation details including AudioContext states, unlock mechanisms, timeout handling, and known quirks
Applied to files:
src/services/audioService.tssrc/services/audioService.test.ts
📚 Learning: 2025-12-15T00:48:06.238Z
Learnt from: CR
Repo: idvorkin/igor-timer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T00:48:06.238Z
Learning: Applies to src/services/pwaDebugServices.ts : Session recording and PWA debug functionality should be implemented in `src/services/pwaDebugServices.ts`
Applied to files:
src/services/audioService.ts
📚 Learning: 2025-12-15T00:48:06.238Z
Learnt from: CR
Repo: idvorkin/igor-timer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T00:48:06.238Z
Learning: Session recordings should capture user clicks/interactions, audio state changes, console errors, and environment info (browser, device)
Applied to files:
src/services/audioService.ts
🔇 Additional comments (12)
src/test/setup.ts (1)
3-4: LGTM!Simple and effective mock to suppress jsdom WebGL warnings. Returning
nullcorrectly simulates an unavailable rendering context.src/services/audioService.test.ts (3)
26-31: LGTM!Clean implementation of event listener tracking that properly handles multiple handlers per event type.
48-55: LGTM!Minimal but sufficient mocks for the silent buffer warmup functionality. Correctly provides the required interface for
createBufferandcreateBufferSource.
202-220: LGTM!Test correctly updated to reflect the new behavior where context destruction occurs on resume timeout. The assertion on
result.errorcontaining "destroyed" aligns with thetestSound()implementation returning"Context state: destroyed".src/services/audioService.ts (8)
41-42: LGTM!Good choice to extract the timeout value as a named constant. 3 seconds is a reasonable timeout for iOS Safari resume operations.
85-108: LGTM!Clean extraction of session recording helpers. Aligns with the coding guidelines about capturing audio state changes in session recordings. Based on learnings, this follows the pattern of recording user interactions and audio state changes.
159-202: Well-structured centralized resume logic.The
attemptResume()method cleanly consolidates the resume flow with proper state checks and error handling. The destruction on failure aligns with the PR objective for iOS recovery.One observation:
playSilentBuffer()is called beforeresume()at line 186. Per iOS audio workaround patterns, the silent buffer playback typically happens to "warm up" the context, but calling it while still suspended might be a no-op on some platforms. Consider verifying this order matches your iOS testing results.
207-216: LGTM!Good implementation of the statechange listener pattern from Howler.js PR #1770. The
audioUnlockedguard correctly prevents premature resume attempts before user gesture.
221-243: LGTM!Clean refactoring of visibility and gesture listeners to use the centralized
attemptResume()method. Thepassive: trueoption on gesture listeners is appropriate for performance.
248-264: LGTM!Good refactoring of
ensureRunning()to use the centralizedattemptResume(). The shared promise pattern correctly prevents race conditions, andgetContext()on line 263 ensures a fresh context is returned if the previous one was destroyed.
269-297: LGTM!Clean refactoring of
playBeep()to useattemptResume(). The fire-and-forget.then()pattern is appropriate for a non-async method, and the null-safe check on line 289 correctly handles the case where context was destroyed.
343-370: LGTM!
testSound()correctly handles the context destruction case, returning"destroyed"state when the context is null after a failed resume attempt. The diagnostic recording at each stage is helpful for debugging.
Improvements for code review: 1. **Fixed timer leak**: withTimeout now clears timer on success 2. **Removed dead code**: Exported helper functions were unused 3. **Better naming**: audioUnlocked → hasUnlockedBefore (clearer intent) 4. **Consolidated flags**: 3 listener flags → 1 listenersAttached 5. **DRY helpers**: getErrorMessage() extracts error messages 6. **Clear sections**: Code organized with section headers 7. **Documented design decisions**: Why playBeep skips vs waits 8. **Improved JSDoc**: All public methods documented with examples 9. **Cleaner event types**: Renamed for consistency (test_played → test_success) Structure: - Types & Constants (top) - Helper functions (pure, testable) - AudioService class with clear sections: - Context Lifecycle - Resume Logic (core attemptResume method) - Event Listeners - Playback - Public API 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Changes
attemptResume()DRY helper (was copy-pasted 6 times)statechangelistener from Howler.js PR #1770destroyContext()on resume failureTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.