AudioManager add suspend#2887
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughA new public static Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 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
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2887 +/- ##
===========================================
- Coverage 83.22% 83.21% -0.02%
===========================================
Files 796 796
Lines 90441 90449 +8
Branches 9498 9498
===========================================
- Hits 75274 75271 -3
- Misses 15085 15096 +11
Partials 82 82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🤖 Fix all issues with AI agents
In `@packages/core/src/audio/AudioManager.ts`:
- Around line 18-20: AudioManager.pause currently calls
AudioManager._context.suspend() and will throw if _context is undefined; change
pause to either guard and return Promise.resolve() when AudioManager._context is
falsy, or call await AudioManager.getContext() (like resume) to ensure
initialization before calling suspend; update the implementation in the
AudioManager.pause method to reference _context and/or getContext accordingly so
it never dereferences undefined.
🧹 Nitpick comments (1)
packages/core/src/audio/AudioManager.ts (1)
13-20: Consider state management for consistency withresume().The
resume()method handles_needsUserGestureResumeflag and deduplicates concurrent calls via_resumePromise. The newpause()method has neither:
- If audio is paused via
pause(), the visibility change handler may still attempt auto-resume when the tab becomes visible (if_playingCount > 0).- Multiple concurrent
pause()calls will each invokesuspend()rather than sharing a single promise.These may be acceptable trade-offs, but worth considering whether
pause()should set a flag to prevent unintended auto-resume.
| static pause(): Promise<void> { | ||
| return AudioManager._context.suspend(); | ||
| } |
There was a problem hiding this comment.
Null reference error if pause() is called before audio context is created.
_context is lazily initialized by getContext(). If pause() is called before any audio operations, _context will be undefined, causing a runtime TypeError.
Consider checking if the context exists, or using getContext() for consistent initialization:
🐛 Proposed fix: guard against uninitialized context
Option 1: Return early if context doesn't exist (no-op when nothing to pause)
static pause(): Promise<void> {
- return AudioManager._context.suspend();
+ const context = AudioManager._context;
+ return context ? context.suspend() : Promise.resolve();
}Option 2: Ensure context is initialized (mirrors resume() behavior)
static pause(): Promise<void> {
- return AudioManager._context.suspend();
+ return AudioManager.getContext().suspend();
}📝 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.
| static pause(): Promise<void> { | |
| return AudioManager._context.suspend(); | |
| } | |
| static pause(): Promise<void> { | |
| const context = AudioManager._context; | |
| return context ? context.suspend() : Promise.resolve(); | |
| } |
🤖 Prompt for AI Agents
In `@packages/core/src/audio/AudioManager.ts` around lines 18 - 20,
AudioManager.pause currently calls AudioManager._context.suspend() and will
throw if _context is undefined; change pause to either guard and return
Promise.resolve() when AudioManager._context is falsy, or call await
AudioManager.getContext() (like resume) to ensure initialization before calling
suspend; update the implementation in the AudioManager.pause method to reference
_context and/or getContext accordingly so it never dereferences undefined.
…ontext terminology
2 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.