fix(playlists): thread the source provider id through to recent playlists - #514
Open
A831ARD0 wants to merge 1 commit into
Open
fix(playlists): thread the source provider id through to recent playlists#514A831ARD0 wants to merge 1 commit into
A831ARD0 wants to merge 1 commit into
Conversation
…ists Fixes spotiflacapp#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 spotiflacapp#368 itself. Verification: go build/vet/test and flutter analyze/test all green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #368.
The bug
Reopening a Spotify playlist from "recent access" showed no tracks, while the first view (right after pasting the URL) worked fine.
Root cause (two layers)
ExtURLHandleResult(the parsed shape of an extension'shandleUrl()return value) never captured a top-levelidfor 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.TrackStatehad nowhere to put that id even if it existed (onlyplaylistName), so recording a "recent" playlist access stored the playlist's name as if it were its id. Reopening it later fed that name intoPlaylistScreen'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.The fix
Matching the "generic API, not per-provider checks" architecture from
CONTRIBUTING.md:go_backend/extension_provider_wrapper.go— addExtURLHandleResult.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— addTrackState.playlistId, populated from the response'sidfield 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 toPlaylistScreen.lib/screens/playlist_screen.dart—PlaylistScreengains ametadataProviderIdparam that takes priority over guessing from the id's shape.lib/screens/home_tab_recent.dart— pass the recent-access entry's storedproviderIdthrough when reopening a playlist.lib/utils/provider_resource_ids.dart— extract the known-id-vs-guessed-id preference into a small, directly testableresolvePreferredMetadataProviderIdhelper.Known limitation (not fixable in this repo)
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
idfor itshandleUrl()playlist result,playlistIdstays 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 newIDfield round-trips from a handler's return value, and stays empty when the handler doesn't supply one.test/provider_resource_ids_test.dart—resolvePreferredMetadataProviderIdprefers 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 [Bug]: Playlists in recent searches do not have tracks #368 itself.Verification
go build ./...,go vet ./...,gofmt -l— cleango test ./...— all green, including the 2 new testsflutter analyze— no issuesflutter test— all 228 tests green, including the 9 new tests