feat(sounds): add provider-backed sound library hook#4540
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rabble
left a comment
There was a problem hiding this comment.
Review summary
Two blocking architecture issues, otherwise the code itself is clean and well-tested. Requesting changes because both issues are direct violations of .claude/rules/ policy that future PRs in this stack will inherit if we land it as-is.
Blocking
1. New feature should be BLoC, not new Riverpod providers.
mobile/lib/providers/sounds_providers.dart adds three brand-new Riverpod providers (soundLibraryApiClientProvider, soundLibraryProvidersProvider, soundLibrarySearchProvider) for a brand-new feature surface. .claude/rules/state_management.md "Migration Policy" is unambiguous:
New features: Use
flutter_blocfollowing the layered architecture (UI → BLoC → Repository → Client)
Existing code: Riverpod is used for legacy code maintenance only
The fact that the existing sounds_providers.dart file already uses Riverpod isn't a license to extend it — that file is legacy. The "hook" in the PR title is doing what a SoundLibraryBloc / SoundLibrarySearchCubit should do (load providers, run a parameterized search). Suggest replacing both FutureProviders with a Cubit emitting a SoundLibrarySearchState (status enum + result list + license metadata).
2. Composition / fallback / provider routing belongs in a repository, not in the client.
.claude/rules/architecture.md:
Fallback and composition logic belongs in the repository — when data can come from multiple sources (e.g., try API first, fall back to local cache or relay), the repository decides the strategy. BLoCs and UI should never implement source-selection or fallback logic.
The design doc explicitly lists divine, nostr, freesound, openverse as providers, and the nostr provider is specced to be "backed by Funnelcake/Nostr reads." That's composition across data sources — the canonical repository responsibility. A SoundsRepository already exists in mobile/packages/sounds_repository/ and is the natural home; the new SoundLibraryApiClient should be one of its data clients, not something the UI talks to directly via a FutureProvider.family. Without this, the eventual nostr provider routing will have to land either in the UI or in the client (which is just an HTTP wrapper today) — both wrong layers.
Concretely: add a method like SoundsRepository.searchProviderSounds(SoundLibrarySearchRequest) that dispatches to SoundLibraryApiClient for external providers and to the existing Nostr path for nostr, and have the new bloc consume the repository.
Nits
- Mixed provider declaration styles in one file. Hand-written
final Provider<X> = Provider<X>((ref) => ...)sits next to@Riverpod(keepAlive: true)codegen providers (soundsRepository,trendingSounds, etc.). The// ignore: specify_nonobvious_property_typesonsoundLibrarySearchProvideris a smell that codegen was the right path. If Riverpod is kept (against finding #1), use@riverpodconsistently. SoundLibrarySearchRequestre-implements==/hashCodeby hand. Five fields, all primitives —Equatableor a record would be shorter and harder to get wrong when fields are added.SoundLibraryApiException implements Exceptionbut the rule inerror_handling.mdis "use descriptive exception classes." Good. But consider whether thecodestrings (invalid_query,provider_disabled, etc.) should be a typed enum so callers can switch on them — string-typed error codes drift between client and proxy._soundFromJsondoes uncheckedas Stringcasts onprovider/providerId/previewUrl/license. If the proxy ever returns a malformed result (or a partially-disabled provider), this throwsTypeErrormid-list-map and the whole search fails. Either validate fields explicitly and skip bad rows (and log), or surface asSoundLibraryApiExceptionso the BLoC canaddErrorit cleanly.- Timeout (12s) and base URL are constructor-injectable but the production
soundLibraryApiClientProviderbuilds with defaults. That's fine for now but worth a// TODO(#issue):if the value is expected to be tuned, percode_style.md"Temporary Code" — or move to aSoundLibraryApiConfigconstant per "No Hardcoded Values."
Things checked and OK
AudioEvent/AudioExternalSource/AudioLicenseMetadatamodel additions look clean: JSON round-trip is tested,copyWithis updated,toJsonomits the new field when null. License isrequiredonAudioExternalSourcewhich matches the proxy contract ("Every returned result must include normalized license metadata or be dropped").- No Riverpod-bridge identity-capture bug here.
SoundLibraryApiClienthas no auth-flippable dependencies;_baseUri/_timeoutare immutable post-construction. TheBlocProvider-keyed-on-identity rule does not apply. - Test coverage on the new client and providers is reasonable —
MockClientexercises both happy and error paths; provider tests useoverrideWithValue. (Once finding #1 is addressed, these need to migrate toblocTest.) - No l10n / theming /
divine_uiissues since this PR is API + model only, no UI yet.
Verdict
Request changes. The two blocking issues are policy violations that get harder to undo once UI lands on top of them — better to flip Riverpod → BLoC and insert the repository layer now, before the audio-picker integration arrives.
…ition Addresses self-review on PR #4540: - Move SoundLibraryApiClient into the sounds_repository package and inject baseUri so the package stays Flutter-free. - Extend SoundsRepository with searchExternalLibrary / fetchExternalProviders so source-selection (divine/nostr/freesound/openverse) lives at the repository layer per architecture.md, not in the BLoC or UI. - Replace soundLibraryApiClientProvider / soundLibraryProvidersProvider / soundLibrarySearchProvider Riverpod providers with SoundLibraryBloc, using enum status + addError (no error strings in state) and restartable query handler so stale searches are cancelled. - Harden _soundFromJson against malformed proxy rows: required string fields and missing license surface as SoundLibraryApiException instead of an opaque TypeError that would abort the whole search. - Add bloc tests, repository routing tests, and a malformed-row regression test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6a9f640 to
5a4a62d
Compare
|
Addressed self-review feedback in 5a4a62d:
Rebased onto fresh |
This comment has been minimized.
This comment has been minimized.
|
CI fix in 82d0cd8:
Verified locally on Flutter 3.41.9 via
|
This comment has been minimized.
This comment has been minimized.
rabble
left a comment
There was a problem hiding this comment.
Re-review of 5a4a62d30 + 82d0cd82f — all three prior blockers resolved.
Riverpod → BLoC migration ✓. mobile/lib/blocs/sound_library/sound_library_bloc.dart ships SoundLibraryBloc with sealed events, enum-status state (SoundLibraryProvidersStatus, SoundLibrarySearchStatus), restartable() on QueryChanged, droppable() on PageRequested, errors via addError() (no error strings/exceptions in state), no mutable instance vars. Old Riverpod hooks fully removed.
Composition moved to repository ✓. SoundLibraryApiClient now lives in mobile/packages/sounds_repository/lib/src/sound_library_api_client.dart (Flutter-free: http + meta only). SoundsRepository.fetchExternalProviders() and .searchExternalLibrary(SoundLibrarySearchRequest) are the BLoC's only surface — client is internal, matching architecture.md.
_soundFromJson hardened ✓. New _requireString helper throws SoundLibraryApiException; the AudioLicenseMetadata.fromJson TypeError is caught and rewrapped with a single annotated // ignore: avoid_catching_errors. Regression test added.
Forward-looking concerns (non-blocking):
- When the UI PR lands, the consuming widget must
ref.watch(soundsRepositoryProvider)and put it in aValueKeyonBlocProvider<SoundLibraryBloc>perstate_management.md"Bridging Riverpod-provided dependencies into BlocProvider." Worth pinning in the design doc. pubkey: AudioEvent.externalProviderMarker('external_provider') stuffs a sentinel into a Nostr-pubkey field — anything callingnpub.encode(pubkey)will choke. Cleaner shape would be a nullablepubkeyor anoriginenum. Flag before any UI readspubkeyof external sounds.- Formatter churn in 6 unrelated
models/files (from pinning Flutter 3.41.9) is mechanically correct but ideally would be a separatechore(models)PR percode_style.md"PR Scope". Acceptable here since the pin lives in this PR.
CI green, sounds_repository at 100% coverage. Verdict: LGTM (posted as comment — GitHub blocks --approve on own PRs).
…ition Addresses self-review on PR #4540: - Move SoundLibraryApiClient into the sounds_repository package and inject baseUri so the package stays Flutter-free. - Extend SoundsRepository with searchExternalLibrary / fetchExternalProviders so source-selection (divine/nostr/freesound/openverse) lives at the repository layer per architecture.md, not in the BLoC or UI. - Replace soundLibraryApiClientProvider / soundLibraryProvidersProvider / soundLibrarySearchProvider Riverpod providers with SoundLibraryBloc, using enum status + addError (no error strings in state) and restartable query handler so stale searches are cancelled. - Harden _soundFromJson against malformed proxy rows: required string fields and missing license surface as SoundLibraryApiException instead of an opaque TypeError that would abort the whole search. - Add bloc tests, repository routing tests, and a malformed-row regression test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This PR pinned the models CI Flutter version to 3.41.9, whose `dart format` is stricter than what `origin/main` shipped with. Six upstream files needed reformatting to make `dart format --set-exit-if-changed lib test` exit 0 under the pinned version. No semantic changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bring SoundLibraryApiClient line coverage from 88.97% to 100% so the package CI's --min-coverage 100 gate passes. Adds tests for: default constructor (auto-instantiated http.Client / timeout), wrapped ClientException, non-JSON success body, non-JSON error body, error body that decodes to a non-Map, empty query rejection, TypeError from license metadata parsing surfacing as a typed API exception, the openverse provider label branch, count-fallback when absent, invalid search/provider response shapes, blank license_type omission. Also adds value-equality, hashCode, and toString coverage for SoundLibraryProviderInfo, SoundLibrarySearchRequest, and SoundLibraryApiException. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
82d0cd8 to
837da32
Compare
|
Pushed a cleanup pass onto What changed:
Local verification after rebase:
I’m watching CI on this head and will fix anything that comes back red. |
Mobile PR PreviewPreview refreshed for Last refresh:
|
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Final pass complete after cleanup. Removed the unshipped app-layer sound library wiring, kept the repository/model groundwork, added value-equality coverage, aligned the sounds repository CI pin with models CI, rebased onto current main, and reran the relevant local verification. Current GitHub checks are green and this is ready to merge.
Closes #4541
Summary
Tests