fix(audio): route extracted audio paths to AudioSourceConfig.file (#4395)#4549
Conversation
) Crashlytics issue e81e2bc1d6713fa5d91e7b15a0950e18: extracted audio paths (bare absolute paths produced by clip_editor_bloc via extractAudio(videoPath)) were classified as .network in AudioTimingCubit and passed to HttpClient.getUrl, crashing with 'No host specified in URI'. Fix: - AudioTimingCubit: three-way classification in _setClippedAudioSource: bundled -> AudioSourceConfig.asset file:// URI or bare absolute path (starts with '/') -> AudioSourceConfig.file (with Uri.toFilePath decoding) otherwise -> AudioSourceConfig.network - AudioClipPlayer: defense-in-depth guard in _defaultRemoteAudioFileLoader throws ArgumentError for non-http(s) schemes. Tests: - 4 new blocTests in audio_timing_cubit_test.dart (bare path, file://, https, bundled). - 2 new tests in audio_clip_player_test.dart (guard throws, file source skips remote loader).
This comment has been minimized.
This comment has been minimized.
…ob: in guard comment - Extract inline URL classification logic in AudioTimingCubit into a private static _configForUrl helper (cleaner _setClippedAudioSource). - Add a note to the ArgumentError guard in AudioClipPlayer that platform-specific schemes (Android content://, web blob:) are also rejected, not just bare paths and file:// URIs.
This comment has been minimized.
This comment has been minimized.
Mobile PR PreviewPreview refreshed for Last refresh:
|
rabble
left a comment
There was a problem hiding this comment.
Review: PR #4549 — fix(audio): route extracted audio paths to AudioSourceConfig.file
Verdict: Approve
Targeted, well-scoped fix for Crashlytics issue #4395 (StateError: No host specified in URI). The diagnosis is correct and traceable end-to-end.
Correctness
- Root cause confirmed.
ClipEditorBloc._onAudioExtractionRequested(line 499–512) constructsAudioEventwithurl: result.audioFilePath, whereaudioFilePathis a bare absolute path like/var/mobile/.../tmp/extracted_audio_<ts>.wavfromAudioExtractionService(line 158).AudioTimingCubit._setClippedAudioSourcepreviously force-routed any non-bundled source throughAudioSourceConfig.network, which_defaultRemoteAudioFileLoaderthen handed toHttpClient.getUrl— boom. - Three-way classification is the right shape.
AudioSourceConfig.filealready exists insound_serviceand routes correctly toAudioSource.file(config.uri)inaudio_clip_player.dart:70.file://URIs are decoded viaUri.toFilePath()(handles URI-encoded path segments correctly). Bare absolute path detection viastartsWith('/')matches the actual extraction output. - Layering is correct. The classification lives in the cubit that knows the
AudioEventcontract — not in the UI, not leaked into the player. The repository/source-of-truth layer here is theAudioEventmodel, and the cubit is the appropriate translator.
Defense-in-depth (good)
AudioClipPlayer._defaultRemoteAudioFileLoadernow throwsArgumentErrorfor non-http(s) schemes with an actionable message namingAudioSourceConfig.file. This is not a silent fallback — it surfaces future regressions loudly at the boundary, which is exactly whaterror_handling.mdasks for. Androidcontent://and webblob:are explicitly called out in the doc comment as falling through to this rejection path; that is a reasonable scoping decision for the current fix.
Test coverage
Strong regression coverage at both layers:
audio_timing_cubit_test.dart: bare absolute path,file://URI, http(s) URL — three newblocTests assertingcaptured.isFile/captured.isAssetandurishape.audio_clip_player_test.dart: assertsArgumentErrorthrown for non-http(s) network config and thatHttpClient.getUrlis never called (verifyNever); separate test assertsAudioSourceConfig.filenever invokes the remote loader at all.
Both layers have an explicit "this must not silently break again" regression test.
Minor observations (non-blocking)
- The bare-absolute-path heuristic (
url.startsWith('/')) is Unix-specific. Web/Windows paths are not affected here because the extracted audio path comes frompath_provider'stempDiron iOS/Android (always starts with/), and the existing in-appAudioEventsources are http(s). If a future caller stuffs a Windows path orcontent://URI intoAudioEvent.url, the defense-in-depthArgumentErrorwill catch it — which is the right failure mode. - The PR description's noted out-of-scope schemes (
content://,blob:) are documented in both the helper's dartdoc and the loader's reject message, so the omission is intentional and traceable.
Project conventions
- No hardcoded strings outside what's already in
AudioEvent. - BLoC-layer logic, no UI leakage.
- Reportable-error policy:
ArgumentErrorfrom a defense-in-depth boundary is appropriate; it signals a programming-invariant violation by the caller (pererror_handling.mdmatrix → reportable). No state pollution with error strings. - Tests follow
blocTest+captureAnymocktail patterns used elsewhere in this file.
Risk
Low. The change is additive in semantics (a previously-broken code path now works) and the network path is unchanged for valid http(s) URLs. CI is green (13/13).
rabble
left a comment
There was a problem hiding this comment.
Re-review of c8dffad: refactor(audio): extract _configForUrl helper
Verdict: Approve (re-approve)
Pure code-organization refactor on top of the already-approved fix. No behavior change, all prior correctness guarantees preserved.
What c8dffad actually does
Two changes, both non-functional:
audio_timing_cubit.dart— extracts the inline URL-classification block from_setClippedAudioSourceinto astatic AudioSourceConfig _configForUrl(String url, {required Duration start, required Duration end})helper. Byte-for-byte identical predicate (parsed == null || parsed.scheme.isEmpty || parsed.scheme == 'file' || url.startsWith('/')), identicalUri.toFilePath()decoding path, identical fallback toAudioSourceConfig.network.audio_clip_player.dart— appends one sentence to the guard's leading comment noting that Androidcontent://and webblob:are rejected by the same scheme check.
That's it. git diff between 3c771d2 and c8dffad touches exactly two files; no test files changed.
Correctness preservation
- Logic equivalence: byte-for-byte same predicate and decoding. Only naming change is
uri->urlparameter; the predicate uses both parsedUri?and raw string identically. Theparsed.scheme == 'file' ? parsed.toFilePath() : urlbranch matches the prior inline form exactly. - Three-way classification still lives at the right layer (the cubit that knows the
AudioEventcontract). Bundled-asset branch is untouched and still constructed inline above the helper call — appropriate, since asset routing needs_sound.assetPathand a different config constructor. - Defense-in-depth boundary in
AudioClipPlayer._defaultRemoteAudioFileLoaderis unchanged; the doc comment addition is documentation-only and accurate (content://andblob:both have non-http(s) schemes and will hit theArgumentErrorbranch).
Test coverage still binds the behavior
The four blocTests in audio_timing_cubit_test.dart (bare path -> .file, file:// -> .file, https -> .network, bundled -> .asset) exercise the same observable contract via cubit.resumePlayback() and the captured setClip argument. They route through _setClippedAudioSource which now calls _configForUrl, so the helper is covered transitively. No separate helper-level unit test was added; given the integration-level coverage exercises the same four branches, that's reasonable.
The two audio_clip_player_test.dart regressions (guard throws on non-http(s); .file skips the remote loader) still anchor the player-layer contract — the doc comment change cannot affect them.
Conventions
- Private
statichelper with focused dartdoc explaining the supported / unsupported scheme matrix — matchescode_style.md(comment the why: the unsupported-scheme fallthrough is intentional and traceable to the defense-in-depthArgumentError). - Helper is at the right granularity: takes only
url,start,end; returns the typed config. Not over-extracted. - No
// TODO, no skipped tests, no behavior shift, no scope creep.
Risk
Zero net new risk. Refactor reduces the cyclomatic complexity of _setClippedAudioSource and makes the doc comment trail (cubit dartdoc -> player guard comment) align across both layers about which schemes are in vs out of scope. Re-approving.
Problem
Crashlytics issue
e81e2bc1d6713fa5d91e7b15a0950e18(16 events, 2 users, v1.0.13):AudioEvent.urlis overloaded: it contains either network URLs or bare absolute file paths (e.g. fromclip_editor_bloc.dartviaextractAudio(videoPath)).AudioTimingCubitclassified everything non-bundled as.network; the default remote loader passed the path without scheme validation toHttpClient.getUrl.Fix
AudioTimingCubit(primary): three-way classification in_setClippedAudioSourceAudioSourceConfig.assetfile://URI or bare absolute path (starts with/) →AudioSourceConfig.file(decoded viaUri.toFilePath())AudioSourceConfig.networkAudioClipPlayer(defense-in-depth):_defaultRemoteAudioFileLoadernow throwsArgumentErrorfor non-http(s) schemes instead of silently passing a broken URI toHttpClient.getUrl.Closes #4395
Type of Change