From bed95efb29d223759ecabeaed2ba44a3e6da05c7 Mon Sep 17 00:00:00 2001 From: Abelardo Ramirez Date: Mon, 3 Aug 2026 09:37:39 -0600 Subject: [PATCH] fix(playlists): thread the source provider id through to recent playlists Fixes #368. Reopening a Spotify playlist from "recent access" showed no tracks, while the first view (right after pasting the URL) worked fine. Root cause traced across two layers: 1. Go: ExtURLHandleResult (the parsed shape of an extension's handleUrl() return value) never captured a top-level `id` for the handled resource. Track/album/artist results carry their own id inside their nested metadata, but a plain playlist result has no such object, so its id was silently dropped everywhere from the goja parser through to the JSON the Dart side receives. 2. Dart: TrackState had nowhere to put that id even if it existed (only playlistName), so recording a "recent" playlist access stored the playlist's *name* as if it were its id. Reopening it later fed that name into PlaylistScreen's provider-guessing logic (legacyProviderIdFromResourceId, which only recognizes legacy "provider:id" prefixes), which naturally failed and fell through to a hardcoded Deezer metadata fetch using a Spotify playlist's name as the resource id - guaranteed to return nothing. Fix, matching the "generic API, not per-provider checks" architecture in CONTRIBUTING.md: - go_backend/extension_provider_wrapper.go: add ExtURLHandleResult.ID. - go_backend/extension_goja_convert.go: parse it from the handler's return value. - go_backend/exports_extensions.go: surface it in the JSON response. - lib/providers/track_provider.dart: add TrackState.playlistId, populated from the response's `id` field for playlist results. - lib/screens/home_tab.dart: record the real playlist id (falling back to the name only if a provider never supplies one) and pass both the id and the already-known provider id forward to PlaylistScreen. - lib/screens/playlist_screen.dart: PlaylistScreen gains a metadataProviderId param that takes priority over guessing from the id's shape. - lib/screens/home_tab_recent.dart: pass the recent-access entry's stored providerId through when reopening a playlist. - lib/utils/provider_resource_ids.dart: extract the known-id-vs-guessed-id preference into a small, directly testable resolvePreferredMetadataProviderId helper. Note: this fixes the common case (an extension already reported its own id as the source provider for the URL it handled). If a provider never supplies an id for its handleUrl() playlist result, playlistId stays null and behavior is unchanged from before this fix - no regression, just not a complete fix for that narrower case, since that would require changes in extension-side JS code outside this repo. Tests: - go_backend/extension_goja_convert_url_handle_test.go: the new ID field round-trips from a handler's return value, and stays empty when the handler doesn't supply one. - test/provider_resource_ids_test.dart: resolvePreferredMetadataProviderId prefers a known provider id, falls back to the legacy-prefix guess, treats a blank known id as unknown, and returns null for the unprefixed-id-with-no-known-provider case from #368 itself. Verification: go build/vet/test and flutter analyze/test all green. --- go_backend/exports_extensions.go | 1 + go_backend/extension_goja_convert.go | 1 + .../extension_goja_convert_url_handle_test.go | 53 +++++++++++++ go_backend/extension_provider_wrapper.go | 6 +- lib/providers/track_provider.dart | 5 ++ lib/screens/home_tab.dart | 7 +- lib/screens/home_tab_recent.dart | 1 + lib/screens/playlist_screen.dart | 7 +- lib/utils/provider_resource_ids.dart | 17 ++++ test/provider_resource_ids_test.dart | 79 +++++++++++++++++++ 10 files changed, 173 insertions(+), 4 deletions(-) create mode 100644 go_backend/extension_goja_convert_url_handle_test.go create mode 100644 test/provider_resource_ids_test.dart diff --git a/go_backend/exports_extensions.go b/go_backend/exports_extensions.go index d15bbb85..b41fd789 100644 --- a/go_backend/exports_extensions.go +++ b/go_backend/exports_extensions.go @@ -805,6 +805,7 @@ func HandleURLWithExtensionJSON(url string) (string, error) { response := map[string]any{ "type": result.Type, + "id": result.ID, "extension_id": extensionID, "name": result.Name, "cover_url": result.CoverURL, diff --git a/go_backend/extension_goja_convert.go b/go_backend/extension_goja_convert.go index 4c87116e..f2e336b0 100644 --- a/go_backend/extension_goja_convert.go +++ b/go_backend/extension_goja_convert.go @@ -522,6 +522,7 @@ func parseExtensionURLHandleValue(vm *goja.Runtime, value goja.Value) (ExtURLHan obj := value.ToObject(vm) handleResult := ExtURLHandleResult{ Type: gojaObjectString(obj, "type"), + ID: gojaObjectString(obj, "id"), Name: gojaObjectString(obj, "name"), CoverURL: gojaObjectString(obj, "cover_url", "coverUrl"), HeaderImage: gojaObjectString(obj, "header_image", "headerImage"), diff --git a/go_backend/extension_goja_convert_url_handle_test.go b/go_backend/extension_goja_convert_url_handle_test.go new file mode 100644 index 00000000..d89da14d --- /dev/null +++ b/go_backend/extension_goja_convert_url_handle_test.go @@ -0,0 +1,53 @@ +package gobackend + +import ( + "testing" + + "github.com/dop251/goja" +) + +func TestParseExtensionURLHandleValueCapturesID(t *testing.T) { + vm := goja.New() + value, err := vm.RunString(`({ + type: "playlist", + id: "37i9dQZF1DXcBWIGoYBM5M", + name: "Discover Weekly", + tracks: [] + })`) + if err != nil { + t.Fatalf("RunString: %v", err) + } + + result, err := parseExtensionURLHandleValue(vm, value) + if err != nil { + t.Fatalf("parseExtensionURLHandleValue: %v", err) + } + if result.Type != "playlist" { + t.Fatalf("Type = %q", result.Type) + } + if result.ID != "37i9dQZF1DXcBWIGoYBM5M" { + t.Fatalf("ID = %q, want the playlist id from the handler result", result.ID) + } + if result.Name != "Discover Weekly" { + t.Fatalf("Name = %q", result.Name) + } +} + +func TestParseExtensionURLHandleValueOmitsIDWhenAbsent(t *testing.T) { + vm := goja.New() + // Track/album/artist results carry their own ID inside their nested + // metadata, so a handler that omits the top-level "id" (as these do + // today) must not surface a stale or zero-value ID. + value, err := vm.RunString(`({ type: "track", name: "A Track" })`) + if err != nil { + t.Fatalf("RunString: %v", err) + } + + result, err := parseExtensionURLHandleValue(vm, value) + if err != nil { + t.Fatalf("parseExtensionURLHandleValue: %v", err) + } + if result.ID != "" { + t.Fatalf("ID = %q, want empty when the handler result has none", result.ID) + } +} diff --git a/go_backend/extension_provider_wrapper.go b/go_backend/extension_provider_wrapper.go index f23f7f0d..84363fdb 100644 --- a/go_backend/extension_provider_wrapper.go +++ b/go_backend/extension_provider_wrapper.go @@ -724,7 +724,11 @@ func (p *extensionProviderWrapper) customSearch(query string, options map[string } type ExtURLHandleResult struct { - Type string `json:"type"` + Type string `json:"type"` + // ID identifies the handled resource itself (e.g. a playlist ID). Track, + // album, and artist results already carry their own ID inside their + // nested metadata; this covers result types with no such nested object. + ID string `json:"id,omitempty"` Track *ExtTrackMetadata `json:"track,omitempty"` Tracks []ExtTrackMetadata `json:"tracks,omitempty"` Album *ExtAlbumMetadata `json:"album,omitempty"` diff --git a/lib/providers/track_provider.dart b/lib/providers/track_provider.dart index e8c3d99a..8f86ce6d 100644 --- a/lib/providers/track_provider.dart +++ b/lib/providers/track_provider.dart @@ -19,6 +19,7 @@ class TrackState { final String? albumId; final String? albumName; final String? playlistName; + final String? playlistId; final String? artistId; final String? artistName; final String? coverUrl; @@ -40,6 +41,7 @@ class TrackState { this.albumId, this.albumName, this.playlistName, + this.playlistId, this.artistId, this.artistName, this.coverUrl, @@ -64,6 +66,7 @@ class TrackState { String? albumId, String? albumName, String? playlistName, + String? playlistId, String? artistId, String? artistName, String? coverUrl, @@ -87,6 +90,7 @@ class TrackState { albumId: albumId ?? this.albumId, albumName: albumName ?? this.albumName, playlistName: playlistName ?? this.playlistName, + playlistId: playlistId ?? this.playlistId, artistId: artistId ?? this.artistId, artistName: artistName ?? this.artistName, coverUrl: coverUrl ?? this.coverUrl, @@ -243,6 +247,7 @@ class TrackNotifier extends Notifier { collectionName ?? (result['album'] as Map?)?['name'] as String?, playlistName: type == 'playlist' ? collectionName : null, + playlistId: type == 'playlist' ? result['id'] as String? : null, coverUrl: normalizeCoverReference(result['cover_url']?.toString()), headerVideoUrl: normalizeRemoteHttpUrl( result['header_video']?.toString(), diff --git a/lib/screens/home_tab.dart b/lib/screens/home_tab.dart index 1ebd0b48..e15e67cc 100644 --- a/lib/screens/home_tab.dart +++ b/lib/screens/home_tab.dart @@ -598,13 +598,14 @@ class _HomeTabState extends ConsumerState } if (trackState.playlistName != null && trackState.tracks.isNotEmpty) { + final playlistProviderId = trackState.searchExtensionId ?? 'spotify'; ref .read(recentAccessProvider.notifier) .recordPlaylistAccess( - id: trackState.playlistName!, + id: trackState.playlistId ?? trackState.playlistName!, name: trackState.playlistName!, imageUrl: trackState.coverUrl, - providerId: trackState.searchExtensionId ?? 'spotify', + providerId: playlistProviderId, ); Navigator.push( @@ -614,6 +615,8 @@ class _HomeTabState extends ConsumerState playlistName: trackState.playlistName!, coverUrl: trackState.coverUrl, tracks: trackState.tracks, + playlistId: trackState.playlistId, + metadataProviderId: playlistProviderId, recommendedService: trackState.searchExtensionId ?? trackState.searchSource, ), diff --git a/lib/screens/home_tab_recent.dart b/lib/screens/home_tab_recent.dart index b07493a2..e81b5f74 100644 --- a/lib/screens/home_tab_recent.dart +++ b/lib/screens/home_tab_recent.dart @@ -342,6 +342,7 @@ extension _HomeTabRecentUI on _HomeTabState { coverUrl: item.imageUrl, tracks: const [], playlistId: item.id, + metadataProviderId: item.providerId, ), ), ); diff --git a/lib/screens/playlist_screen.dart b/lib/screens/playlist_screen.dart index 334145c4..a49097ab 100644 --- a/lib/screens/playlist_screen.dart +++ b/lib/screens/playlist_screen.dart @@ -31,6 +31,7 @@ class PlaylistScreen extends ConsumerStatefulWidget { final String? headerVideoUrl; final List tracks; final String? playlistId; + final String? metadataProviderId; final String? recommendedService; const PlaylistScreen({ @@ -40,6 +41,7 @@ class PlaylistScreen extends ConsumerStatefulWidget { this.headerVideoUrl, required this.tracks, this.playlistId, + this.metadataProviderId, this.recommendedService, }); @@ -126,7 +128,10 @@ class _PlaylistScreenState extends ConsumerState } String? _metadataProviderId(String playlistId) { - final providerId = legacyProviderIdFromResourceId(playlistId); + final providerId = resolvePreferredMetadataProviderId( + widget.metadataProviderId, + playlistId, + ); if (providerId == null) return null; final effective = resolveEffectiveMetadataProvider( providerId, diff --git a/lib/utils/provider_resource_ids.dart b/lib/utils/provider_resource_ids.dart index 6644afc8..8dcea615 100644 --- a/lib/utils/provider_resource_ids.dart +++ b/lib/utils/provider_resource_ids.dart @@ -18,3 +18,20 @@ String stripPrefixedResourceId(String value) { } return value.substring(colonIndex + 1); } + +/// Resolves the metadata provider id to fetch a resource from, preferring a +/// caller-supplied [knownProviderId] (e.g. from a recent-access entry that +/// already recorded its source) over guessing from [resourceId]'s shape. +/// +/// [legacyProviderIdFromResourceId] only recognizes legacy-prefixed ids +/// ("deezer:...", "spotify:...", etc.); anything else - including a plain, +/// unprefixed id from a provider that never used that scheme - resolves to +/// null unless the caller already knows the provider. +String? resolvePreferredMetadataProviderId( + String? knownProviderId, + String resourceId, +) { + final known = knownProviderId?.trim(); + if (known != null && known.isNotEmpty) return known; + return legacyProviderIdFromResourceId(resourceId); +} diff --git a/test/provider_resource_ids_test.dart b/test/provider_resource_ids_test.dart new file mode 100644 index 00000000..2177a29f --- /dev/null +++ b/test/provider_resource_ids_test.dart @@ -0,0 +1,79 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:spotiflac_android/constants/music_services.dart'; +import 'package:spotiflac_android/utils/provider_resource_ids.dart'; + +void main() { + group('legacyProviderIdFromResourceId', () { + test('maps known legacy prefixes to their provider id', () { + expect( + legacyProviderIdFromResourceId('deezer:123'), + MusicServices.deezer, + ); + expect(legacyProviderIdFromResourceId('qobuz:123'), MusicServices.qobuz); + expect(legacyProviderIdFromResourceId('tidal:123'), MusicServices.tidal); + expect( + legacyProviderIdFromResourceId('spotify:123'), + MusicServices.spotify, + ); + }); + + test('returns null for an id with no known prefix', () { + expect(legacyProviderIdFromResourceId('37i9dQZF1DXcBWIGoYBM5M'), isNull); + expect(legacyProviderIdFromResourceId(''), isNull); + }); + }); + + group('stripPrefixedResourceId', () { + test('strips a leading provider prefix', () { + expect(stripPrefixedResourceId('deezer:123'), '123'); + }); + + test('leaves an id without a prefix unchanged', () { + expect( + stripPrefixedResourceId('37i9dQZF1DXcBWIGoYBM5M'), + '37i9dQZF1DXcBWIGoYBM5M', + ); + }); + + test('leaves a bare colon or trailing colon unchanged', () { + expect(stripPrefixedResourceId(':123'), ':123'); + expect(stripPrefixedResourceId('deezer:'), 'deezer:'); + }); + }); + + group('resolvePreferredMetadataProviderId', () { + test('prefers a known provider id over guessing from the resource id', () { + expect( + resolvePreferredMetadataProviderId( + 'apple-music-ext', + '37i9dQZF1DXcBWIGoYBM5M', + ), + 'apple-music-ext', + ); + }); + + test('falls back to the legacy prefix guess when nothing is known', () { + expect( + resolvePreferredMetadataProviderId(null, 'deezer:123'), + MusicServices.deezer, + ); + }); + + test('treats a blank known provider id as unknown', () { + expect( + resolvePreferredMetadataProviderId(' ', 'deezer:123'), + MusicServices.deezer, + ); + }); + + test( + 'returns null for an unprefixed id with no known provider (the #368 case)', + () { + expect( + resolvePreferredMetadataProviderId(null, '37i9dQZF1DXcBWIGoYBM5M'), + isNull, + ); + }, + ); + }); +}