Fix audio catch error #2876
Fix audio catch error #2876
Conversation
WalkthroughReordered AudioLoader flow to create the AudioClip after fetching the ArrayBuffer and added an outer catch for request failures. Also added a test case to verify load error behavior for a network 404 and a TS-ignore to bypass environment-specific type checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Loader as AudioLoader
participant Network as _request (ArrayBuffer)
participant Decoder as decodeAudioData
participant Clip as AudioClip
Client->>Loader: load(url)
Loader->>Network: fetch ArrayBuffer
alt request succeeds
Network-->>Loader: ArrayBuffer
Loader->>Clip: instantiate AudioClip (after data)
Loader->>Decoder: decodeAudioData(ArrayBuffer)
alt decode succeeds
Decoder-->>Loader: AudioBuffer
Loader->>Clip: set source, assign name
Loader-->>Client: resolve(AudioClip)
else decode fails
Decoder-->>Loader: reject(error)
Loader-->>Client: reject(error)
end
else request fails
Network-->>Loader: reject(error)
Loader-->>Client: reject(error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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 |
🤖 Augment PR SummarySummary: This PR fixes error propagation in Changes:
Technical Notes: Ensures both network/request errors and decode errors end up calling 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2876 +/- ##
==========================================
+ Coverage 78.03% 78.53% +0.49%
==========================================
Files 867 867
Lines 94347 94353 +6
Branches 9401 9398 -3
==========================================
+ Hits 73626 74099 +473
+ Misses 20560 20093 -467
Partials 161 161
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/loader/src/AudioLoader.ts`:
- Around line 25-43: The AudioClip created in AudioLoader.ts (new
AudioClip(resourceManager.engine) assigned to audioClip) is left registered if
AudioManager.getContext().decodeAudioData fails; update the
decodeAudioData.catch handler to perform the same cleanup used elsewhere (call
the AudioClip's destroy/cleanup method to unregister the resource) before
rejecting the promise so the ReferResource registration is removed; locate the
catch inside AudioManager.getContext().decodeAudioData(...) and add audioClip
destruction (e.g., audioClip.destroy() or the project's equivalent) then
reject(e).
🧹 Nitpick comments (1)
packages/loader/src/AudioLoader.ts (1)
22-47: Consider flattening the nested promise chain for improved readability.The current nested
then/catchstructure could be flattened to improve code maintainability. While the current implementation is correct, a flatter structure would be easier to read and maintain.♻️ Optional refactor: Flatten promise chain
- resourceManager - // `@ts-ignore` - ._request<ArrayBuffer>(url, requestConfig) - .then((arrayBuffer) => { - const audioClip = new AudioClip(resourceManager.engine); - // `@ts-ignore` - AudioManager.getContext() - .decodeAudioData(arrayBuffer) - .then((result: AudioBuffer) => { - // `@ts-ignore` - audioClip._setAudioSource(result); - - if (url.indexOf("data:") !== 0) { - const index = url.lastIndexOf("/"); - audioClip.name = url.substring(index + 1); - } - - resolve(audioClip); - }) - .catch((e) => { - reject(e); - }); - }) - .catch((e) => { - reject(e); - }); + resourceManager + // `@ts-ignore` + ._request<ArrayBuffer>(url, requestConfig) + .then((arrayBuffer) => { + const audioClip = new AudioClip(resourceManager.engine); + // `@ts-ignore` + return AudioManager.getContext() + .decodeAudioData(arrayBuffer) + .then((result: AudioBuffer) => { + // `@ts-ignore` + audioClip._setAudioSource(result); + + if (url.indexOf("data:") !== 0) { + const index = url.lastIndexOf("/"); + audioClip.name = url.substring(index + 1); + } + + return audioClip; + }); + }) + .then(resolve) + .catch(reject);This eliminates nested catch blocks and makes the error flow clearer.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loader/src/AudioLoader.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/loader/src/AudioLoader.ts (4)
packages/core/src/Engine.ts (1)
resourceManager(163-165)packages/core/src/audio/AudioClip.ts (1)
AudioClip(7-58)packages/core/src/audio/AudioManager.ts (1)
AudioManager(4-49)packages/core/src/asset/AssetPromise.ts (1)
resolve(25-35)
⏰ 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). (5)
- GitHub Check: e2e (22.x, 2/4)
- GitHub Check: e2e (22.x, 1/4)
- GitHub Check: e2e (22.x, 3/4)
- GitHub Check: e2e (22.x, 4/4)
- GitHub Check: codecov
🔇 Additional comments (2)
packages/loader/src/AudioLoader.ts (2)
45-47: LGTM! Error handling fix successfully addresses the PR objective.The outer catch block now properly handles request failures that were previously uncaught, directly fixing the issue described in the PR where "errors cannot be caught in AudioLoader."
23-23: @ts-ignore usage is necessary for intentional internal APIs.The three
@ts-ignorecomments suppress access to intentional framework-internal APIs that are explicitly marked@internal:
- Line 23:
_request(ResourceManager)- Line 27:
AudioManager.getContext()- Line 31:
_setAudioSource(AudioClip)Since TypeScript doesn't natively support
@internaldecorators,@ts-ignoreis the necessary workaround for accessing these documented internal APIs. This is an established pattern throughout the loader package.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| .then((arrayBuffer) => { | ||
| const audioClip = new AudioClip(resourceManager.engine); | ||
| // @ts-ignore | ||
| AudioManager.getContext() | ||
| .decodeAudioData(arrayBuffer) | ||
| .then((result: AudioBuffer) => { | ||
| // @ts-ignore | ||
| audioClip._setAudioSource(result); | ||
|
|
||
| if (url.indexOf("data:") !== 0) { | ||
| const index = url.lastIndexOf("/"); | ||
| audioClip.name = url.substring(index + 1); | ||
| } | ||
| if (url.indexOf("data:") !== 0) { | ||
| const index = url.lastIndexOf("/"); | ||
| audioClip.name = url.substring(index + 1); | ||
| } | ||
|
|
||
| resolve(audioClip); | ||
| }) | ||
| .catch((e) => { | ||
| reject(e); | ||
| }); | ||
| }); | ||
| resolve(audioClip); | ||
| }) | ||
| .catch((e) => { | ||
| reject(e); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find AudioClip class definition
fd AudioClip.tsRepository: galacean/engine
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Find ReferResource class definition
fd -t f ReferResource.ts -o -name "*ReferResource*"Repository: galacean/engine
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Search for AudioClip class and its inheritance
rg -t ts "class AudioClip" -A 5Repository: galacean/engine
Length of output: 419
🏁 Script executed:
#!/bin/bash
# Find and examine ReferResource class
fd ReferResource.ts | head -1 | xargs catRepository: galacean/engine
Length of output: 2666
🏁 Script executed:
#!/bin/bash
# Check AudioClip class for destroy method and full implementation
cat packages/core/src/audio/AudioClip.tsRepository: galacean/engine
Length of output: 1109
🏁 Script executed:
#!/bin/bash
# Search for decodeAudioData usage patterns in the codebase
rg "decodeAudioData" -B 3 -A 5Repository: galacean/engine
Length of output: 3487
🏁 Script executed:
#!/bin/bash
# Check if there are other AudioLoader error handling patterns or similar resource creation patterns
rg "new AudioClip|new.*extends ReferResource" -B 2 -A 8 packages/loader/Repository: galacean/engine
Length of output: 867
🏁 Script executed:
#!/bin/bash
# Search for destroy() calls in error handlers in the codebase to see the pattern
rg "\.destroy\(\)" -B 3 -A 1 | grep -A 4 -B 4 "catch\|error"Repository: galacean/engine
Length of output: 1590
Add cleanup in the decode error handler to prevent resource leak.
The AudioClip instance created on line 26 is registered with the engine's resource manager via ReferResource constructor, but is not destroyed if decodeAudioData fails. This leaves the resource registered without being properly cleaned up, causing a memory leak.
The suggested fix should be applied. This pattern is already used elsewhere in the codebase (e.g., GLTFLoader.ts):
🔧 Fix
.catch((e) => {
+ audioClip.destroy();
reject(e);
});🤖 Prompt for AI Agents
In `@packages/loader/src/AudioLoader.ts` around lines 25 - 43, The AudioClip
created in AudioLoader.ts (new AudioClip(resourceManager.engine) assigned to
audioClip) is left registered if AudioManager.getContext().decodeAudioData
fails; update the decodeAudioData.catch handler to perform the same cleanup used
elsewhere (call the AudioClip's destroy/cleanup method to unregister the
resource) before rejecting the promise so the ReferResource registration is
removed; locate the catch inside AudioManager.getContext().decodeAudioData(...)
and add audioClip destruction (e.g., audioClip.destroy() or the project's
equivalent) then reject(e).
3 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix
What is the current behavior? (You can also link to an open issue here)
Can't catch error in AudioLoader
What is the new behavior (if this is a feature change)?
Catch erro now
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
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.