fix(audio): guard suspend/resume against uninitialized AudioContext#2963
fix(audio): guard suspend/resume against uninitialized AudioContext#2963cptbtptpbcptdtptp merged 2 commits intodev/2.0from
Conversation
AudioManager._context is lazily created on first audio play. If suspend() or resume() is called before that (e.g. editor visibilitychange handler), it crashes with "Cannot read properties of undefined". Return a resolved promise when _context doesn't exist yet — no AudioContext means nothing to suspend/resume.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2963 +/- ##
===========================================
- Coverage 77.61% 77.56% -0.05%
===========================================
Files 900 900
Lines 98724 98724
Branches 9817 9817
===========================================
- Hits 76621 76573 -48
- Misses 21937 21985 +48
Partials 166 166
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:
|
…sume Ensures AudioContext is created if needed so the state always matches user intent — calling suspend() guarantees audio is actually suspended
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
将 suspend()/resume() 中的 AudioManager._context 直接访问改为 AudioManager.getContext() 调用。改动方向是正确的——避免 _context 为 null 时崩溃。但需要注意 getContext() 是 lazy initializer(不存在时会 new AudioContext()),所以这个改动实际上把语义从"直接访问可能为 null 的 context"变成了"如果 context 不存在就创建一个"。
问题
P1
-
[P1] AudioManager.ts:18,27 —
getContext()会强制创建 AudioContext,而非安全跳过PR 描述说"return
Promise.resolve()when_contextdoesn't exist yet",但实际代码调用getContext()会new AudioContext()+ 注册visibilitychange/touchstart/touchend/click监听器。这意味着:- 用户在无任何音频播放意图时调用
suspend()→ 创建了一个 AudioContext → 浏览器控制台可能弹出 "AudioContext was not allowed to start" 警告(非用户手势触发时) - 注册了不必要的全局事件监听
- 与 PR 描述的行为("No AudioContext means nothing to suspend/resume")不一致
如果目标是 null 安全,应该真正检查
_context是否存在,而不是强制创建:static suspend(): Promise<void> { const context = AudioManager._context; return context ? context.suspend() : Promise.resolve(); } static resume(): Promise<void> { const context = AudioManager._context; if (!context) return Promise.resolve(); return (AudioManager._resumePromise ??= context .resume() .then(() => { AudioManager._needsUserGestureResume = false; }) .finally(() => { AudioManager._resumePromise = null; })); }
这才是 PR 标题 "guard suspend/resume against uninitialized AudioContext" 的正确实现
- 用户在无任何音频播放意图时调用
Summary
AudioManager._contextis lazily created on first audio play, butsuspend()/resume()as public API can be called before thatPromise.resolve()when_contextdoesn't exist yetTest plan
AudioManager.suspend()before any audio plays — should not throwAudioManager.resume()before any audio plays — should not throwSummary by CodeRabbit