From 33b11a3b3dd6565343a46d7950567dcff90691c2 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 14 May 2026 22:26:22 -0400 Subject: [PATCH 01/27] feat(settings): About section with version + force update check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an About card to Settings that shows the installed version (version+build from PackageInfo), the latest known version from clientUpdateProvider, and a "Check for updates" button that invalidates the provider to force a fresh poll. When an update is available, surfaces an Install CTA that reuses the same installer flow as the top banner. The existing banner (shouldShowUpdateBannerProvider) is unaffected — it gates on per-version dismissal, while the About section always reflects the current provider state regardless of dismissal. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../lib/settings/about_section.dart | 227 ++++++++++++++++++ .../lib/settings/settings_screen.dart | 3 + 2 files changed, 230 insertions(+) create mode 100644 flutter_client/lib/settings/about_section.dart diff --git a/flutter_client/lib/settings/about_section.dart b/flutter_client/lib/settings/about_section.dart new file mode 100644 index 00000000..efd5ada1 --- /dev/null +++ b/flutter_client/lib/settings/about_section.dart @@ -0,0 +1,227 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:package_info_plus/package_info_plus.dart'; + +import '../theme/theme_extension.dart'; +import '../update/client_update_provider.dart'; +import '../update/update_info.dart'; + +final _packageInfoProvider = FutureProvider((_) { + return PackageInfo.fromPlatform(); +}); + +class AboutSection extends ConsumerStatefulWidget { + const AboutSection({super.key}); + + @override + ConsumerState createState() => _AboutSectionState(); +} + +enum _InstallStage { idle, downloading, error } + +class _AboutSectionState extends ConsumerState { + DateTime? _lastChecked; + bool _checking = false; + + _InstallStage _installStage = _InstallStage.idle; + double _installProgress = 0; + String? _installError; + + Future _checkNow() async { + setState(() => _checking = true); + ref.invalidate(clientUpdateProvider); + try { + await ref.read(clientUpdateProvider.future); + } finally { + if (mounted) { + setState(() { + _checking = false; + _lastChecked = DateTime.now(); + }); + } + } + } + + Future _install(UpdateInfo info) async { + setState(() { + _installStage = _InstallStage.downloading; + _installProgress = 0; + _installError = null; + }); + try { + final installer = await ref.read(updateInstallerProvider.future); + final path = await installer.download( + info.apkUrl, + onProgress: (p) { + if (mounted) setState(() => _installProgress = p); + }, + ); + await installer.install(path); + } catch (e) { + if (!mounted) return; + setState(() { + _installStage = _InstallStage.error; + _installError = '$e'; + }); + } + } + + @override + Widget build(BuildContext context) { + final fs = Theme.of(context).extension()!; + final pkg = ref.watch(_packageInfoProvider); + final update = ref.watch(clientUpdateProvider); + + final installed = pkg.value == null + ? '…' + : '${pkg.value!.version}+${pkg.value!.buildNumber}'; + + final UpdateInfo? available = update.value; + final hasUpdate = available != null; + + return Column(crossAxisAlignment: CrossAxisAlignment.start, children: [ + const Padding( + padding: EdgeInsets.fromLTRB(16, 4, 16, 8), + child: _SectionHeader('About'), + ), + Padding( + padding: const EdgeInsets.fromLTRB(16, 0, 16, 4), + child: Row( + children: [ + Icon(Icons.info_outline, color: fs.parchment, size: 18), + const SizedBox(width: 8), + Text('Installed version', + style: TextStyle(color: fs.parchment, fontSize: 14)), + const Spacer(), + Text(installed, + style: TextStyle( + color: fs.ash, + fontSize: 13, + fontFamily: 'JetBrainsMono')), + ], + ), + ), + Padding( + padding: const EdgeInsets.fromLTRB(16, 4, 16, 4), + child: Row( + children: [ + Icon(Icons.cloud_outlined, color: fs.parchment, size: 18), + const SizedBox(width: 8), + Text('Latest version', + style: TextStyle(color: fs.parchment, fontSize: 14)), + const Spacer(), + Text( + hasUpdate ? available.version : _statusFor(update, installed), + style: TextStyle( + color: hasUpdate ? fs.accent : fs.ash, + fontSize: 13, + fontFamily: 'JetBrainsMono'), + ), + ], + ), + ), + if (_lastChecked != null) + Padding( + padding: const EdgeInsets.fromLTRB(16, 4, 16, 0), + child: Text( + 'Last checked ${_formatTime(_lastChecked!)}', + style: TextStyle(color: fs.ash, fontSize: 11), + ), + ), + Padding( + padding: const EdgeInsets.fromLTRB(16, 12, 16, 0), + child: Row( + children: [ + FilledButton.icon( + key: const Key('about_check_for_updates'), + onPressed: _checking ? null : _checkNow, + style: FilledButton.styleFrom( + backgroundColor: fs.accent, + foregroundColor: fs.parchment, + ), + icon: _checking + ? SizedBox( + width: 14, + height: 14, + child: CircularProgressIndicator( + strokeWidth: 2, + valueColor: AlwaysStoppedAnimation(fs.parchment), + ), + ) + : const Icon(Icons.refresh, size: 18), + label: Text(_checking ? 'Checking…' : 'Check for updates'), + ), + if (hasUpdate) ...[ + const SizedBox(width: 12), + FilledButton.icon( + key: const Key('about_install_update'), + onPressed: _installStage == _InstallStage.downloading + ? null + : () => _install(available), + style: FilledButton.styleFrom( + backgroundColor: fs.moss, + foregroundColor: fs.parchment, + ), + icon: _installStage == _InstallStage.downloading + ? SizedBox( + width: 14, + height: 14, + child: CircularProgressIndicator( + strokeWidth: 2, + value: _installProgress > 0 ? _installProgress : null, + valueColor: AlwaysStoppedAnimation(fs.parchment), + ), + ) + : const Icon(Icons.system_update, size: 18), + label: Text( + _installStage == _InstallStage.downloading + ? 'Downloading…' + : _installStage == _InstallStage.error + ? 'Retry install' + : 'Install ${available.version}', + ), + ), + ], + ], + ), + ), + if (_installStage == _InstallStage.error && _installError != null) + Padding( + padding: const EdgeInsets.fromLTRB(16, 8, 16, 0), + child: Text(_installError!, + style: TextStyle(color: fs.error, fontSize: 12), + maxLines: 2, + overflow: TextOverflow.ellipsis), + ), + ]); + } + + String _statusFor(AsyncValue update, String installed) { + if (update.isLoading) return 'Checking…'; + if (update.hasError) return 'Check failed'; + return 'Up to date'; + } + + String _formatTime(DateTime t) { + final h = t.hour.toString().padLeft(2, '0'); + final m = t.minute.toString().padLeft(2, '0'); + return '$h:$m'; + } +} + +class _SectionHeader extends StatelessWidget { + const _SectionHeader(this.label); + final String label; + @override + Widget build(BuildContext context) { + final fs = Theme.of(context).extension()!; + return Text( + label, + style: TextStyle( + color: fs.parchment, + fontSize: 16, + fontWeight: FontWeight.w500, + ), + ); + } +} diff --git a/flutter_client/lib/settings/settings_screen.dart b/flutter_client/lib/settings/settings_screen.dart index d1bdbab9..e8810062 100644 --- a/flutter_client/lib/settings/settings_screen.dart +++ b/flutter_client/lib/settings/settings_screen.dart @@ -9,6 +9,7 @@ import '../library/library_providers.dart' show dioProvider; import '../models/my_profile.dart'; import '../shared/widgets/main_app_bar_actions.dart'; import '../theme/theme_extension.dart'; +import 'about_section.dart'; import 'storage_section.dart'; import '../theme/theme_mode_provider.dart'; @@ -56,6 +57,8 @@ class SettingsScreen extends ConsumerWidget { _PasswordSection(), _Divider(), _ListenBrainzSection(), + _Divider(), + AboutSection(), _AdminSection(), SizedBox(height: 96), ], From d12afdad6ee08455b834db1fdac3b33fa197ebd6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 14 May 2026 22:55:57 -0400 Subject: [PATCH 02/27] feat(playlists): system playlists default to shuffle on tile play Closes Fable #412 (For You force-refresh on play) and #413 (shuffle-on-play default for system playlists). Web (PlaylistCard.svelte + player/store.svelte.ts): - Drop the refreshForYou() call from the play handler. The daily 03:00 user-local snapshot is what plays now. Stops burning server compute on every press and stops swapping the playlist out from under the user. - Generalize the kebab affordance to render for any system playlist (was Discover-only). Adds "Refresh For You" as an explicit replacement so users can still force a regen when they want one. - Extend playQueue(tracks, startIndex, { shuffle? }) to Fisher-Yates the queue when shuffle:true. PlaylistCard passes shuffle:true for any non-null system_variant. Flutter (player_provider.dart + playlists/widgets/playlist_card.dart): - playTracks now accepts shuffle:bool. When true, picks a random starting index and enables AudioServiceShuffleMode.all after setQueueFromTracks. PlaylistCard passes shuffle:playlist.isSystem. User playlists keep linear order. Detail-screen play buttons are unchanged for now (follow-up if user requests). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../lib/player/player_provider.dart | 20 +++- .../lib/playlists/widgets/playlist_card.dart | 9 +- web/src/lib/components/PlaylistCard.svelte | 47 ++++---- web/src/lib/components/PlaylistCard.test.ts | 100 +++++++++--------- web/src/lib/player/store.svelte.ts | 31 ++++-- 5 files changed, 124 insertions(+), 83 deletions(-) diff --git a/flutter_client/lib/player/player_provider.dart b/flutter_client/lib/player/player_provider.dart index 14f4afe5..cc14d645 100644 --- a/flutter_client/lib/player/player_provider.dart +++ b/flutter_client/lib/player/player_provider.dart @@ -1,3 +1,5 @@ +import 'dart:math' show Random; + import 'package:audio_service/audio_service.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; @@ -65,7 +67,18 @@ class PlayerActions { } final Ref _ref; - Future playTracks(List tracks, {int initialIndex = 0}) async { + Future playTracks( + List tracks, { + int initialIndex = 0, + bool shuffle = false, + }) async { + // shuffle=true means "play this pool randomly, starting at a + // random track." Used for system playlists so the first track of + // a re-play within the day is different from the prior one. + var startAt = initialIndex; + if (shuffle && tracks.length > 1) { + startAt = Random().nextInt(tracks.length); + } final url = await _ref.read(serverUrlProvider.future); final token = await _ref.read(secureStorageProvider).read(key: 'session_token'); final cache = _ref.read(albumCoverCacheProvider); @@ -78,7 +91,10 @@ class PlayerActions { audioCacheManager: audioCache, likeBridge: _buildLikeBridge(), ); - await h.setQueueFromTracks(tracks, initialIndex: initialIndex); + await h.setQueueFromTracks(tracks, initialIndex: startAt); + if (shuffle) { + await h.setShuffleMode(AudioServiceShuffleMode.all); + } await h.play(); } diff --git a/flutter_client/lib/playlists/widgets/playlist_card.dart b/flutter_client/lib/playlists/widgets/playlist_card.dart index 47e75033..66566558 100644 --- a/flutter_client/lib/playlists/widgets/playlist_card.dart +++ b/flutter_client/lib/playlists/widgets/playlist_card.dart @@ -120,6 +120,13 @@ class PlaylistCard extends ConsumerWidget { )); } if (refs.isEmpty) return; - await ref.read(playerActionsProvider).playTracks(refs, initialIndex: 0); + // System playlists default to shuffle so replaying within a day + // doesn't deliver the identical experience. User playlists keep + // stored order. + await ref.read(playerActionsProvider).playTracks( + refs, + initialIndex: 0, + shuffle: playlist.isSystem, + ); } } diff --git a/web/src/lib/components/PlaylistCard.svelte b/web/src/lib/components/PlaylistCard.svelte index 45f58e22..78c64bb7 100644 --- a/web/src/lib/components/PlaylistCard.svelte +++ b/web/src/lib/components/PlaylistCard.svelte @@ -14,6 +14,11 @@ const isOwn = $derived(user.value?.id === playlist.user_id); const isDiscover = $derived(playlist.system_variant === 'discover'); + const isForYou = $derived(playlist.system_variant === 'for_you'); + const isSystem = $derived(playlist.system_variant != null); + const refreshLabel = $derived( + isDiscover ? 'Refresh Discover' : isForYou ? 'Refresh For You' : 'Refresh', + ); const queryClient = useQueryClient(); @@ -40,38 +45,34 @@ if (starting) return; starting = true; try { - let detailID = playlist.id; - // For-You: refresh first so click means "give me a fresh mix - // and play it." Atomic replace creates a new playlist row, so - // the response's playlist_id is what we follow up with. - if (playlist.system_variant === 'for_you') { - const refreshed = await refreshForYou(); - if (!refreshed.playlist_id) { - // Empty library — nothing to play. - return; - } - detailID = refreshed.playlist_id; - // Invalidate caches so the home + detail views see the new - // playlist on their next read. - await queryClient.invalidateQueries({ queryKey: qk.playlists() }); - } - const detail = await getPlaylist(detailID); + const detail = await getPlaylist(playlist.id); const refs = toTrackRefs(detail.tracks); if (refs.length > 0) { - playQueue(refs, 0); + // System playlists default to shuffle so replaying within a + // day feels different. User playlists keep stored order. + // Explicit refresh moved to the kebab menu — playing no + // longer rebuilds the snapshot. + playQueue(refs, 0, { shuffle: playlist.system_variant != null }); } } finally { starting = false; } } - async function onRefreshDiscover(e: MouseEvent) { + async function onRefresh(e: MouseEvent) { e.preventDefault(); e.stopPropagation(); menuOpen = false; try { - await refreshDiscover(); - pushToast('Discover refreshed.'); + if (isDiscover) { + await refreshDiscover(); + pushToast('Discover refreshed.'); + } else if (isForYou) { + await refreshForYou(); + pushToast('For You refreshed.'); + } else { + return; + } await queryClient.invalidateQueries({ queryKey: qk.playlist(playlist.id) }); await queryClient.invalidateQueries({ queryKey: qk.playlists() }); } catch (err: unknown) { @@ -83,7 +84,7 @@ { menuOpen = false; }} />
- {#if isDiscover} + {#if isSystem}
{/if} diff --git a/web/src/lib/components/PlaylistCard.test.ts b/web/src/lib/components/PlaylistCard.test.ts index 1213c5f9..688f86d9 100644 --- a/web/src/lib/components/PlaylistCard.test.ts +++ b/web/src/lib/components/PlaylistCard.test.ts @@ -143,7 +143,7 @@ describe('PlaylistCard', () => { expect(playQueue).toHaveBeenCalled(); }); - test('shows kebab button only on Discover playlists', () => { + test('shows kebab button on Discover playlists', () => { const discoverPlaylist: Playlist = { ...base, kind: 'system', @@ -154,7 +154,7 @@ describe('PlaylistCard', () => { expect(screen.getByLabelText(/playlist actions/i)).toBeInTheDocument(); }); - test('does not show kebab button on for_you playlists', () => { + test('shows kebab button on For You playlists', () => { const forYouPlaylist: Playlist = { ...base, kind: 'system', @@ -162,7 +162,7 @@ describe('PlaylistCard', () => { name: 'For You' }; render(PlaylistCard, { props: { playlist: forYouPlaylist } }); - expect(screen.queryByLabelText(/playlist actions/i)).not.toBeInTheDocument(); + expect(screen.getByLabelText(/playlist actions/i)).toBeInTheDocument(); }); test('does not show kebab button on user playlists', () => { @@ -170,7 +170,7 @@ describe('PlaylistCard', () => { expect(screen.queryByLabelText(/playlist actions/i)).not.toBeInTheDocument(); }); - test('Refresh Discover item appears in kebab menu when opened', async () => { + test('Refresh Discover item appears in Discover kebab', async () => { const discoverPlaylist: Playlist = { ...base, kind: 'system', @@ -183,7 +183,7 @@ describe('PlaylistCard', () => { expect(screen.getByRole('menuitem', { name: /refresh discover/i })).toBeInTheDocument(); }); - test('Refresh Discover item is absent from for_you card kebab', () => { + test('Refresh For You item appears in For You kebab', async () => { const forYouPlaylist: Playlist = { ...base, kind: 'system', @@ -191,7 +191,9 @@ describe('PlaylistCard', () => { name: 'For You' }; render(PlaylistCard, { props: { playlist: forYouPlaylist } }); - expect(screen.queryByRole('menuitem', { name: /refresh discover/i })).not.toBeInTheDocument(); + const kebabBtn = screen.getByLabelText(/playlist actions/i); + await fireEvent.click(kebabBtn); + expect(screen.getByRole('menuitem', { name: /refresh for you/i })).toBeInTheDocument(); }); test('clicking Refresh Discover calls refreshDiscover', async () => { @@ -210,7 +212,23 @@ describe('PlaylistCard', () => { await waitFor(() => expect(refreshDiscover).toHaveBeenCalled()); }); - test('For-You play button calls refreshForYou before getPlaylist', async () => { + test('clicking Refresh For You calls refreshForYou', async () => { + const { refreshForYou } = await import('$lib/api/playlists'); + const forYouPlaylist: Playlist = { + ...base, + kind: 'system', + system_variant: 'for_you', + name: 'For You' + }; + render(PlaylistCard, { props: { playlist: forYouPlaylist } }); + const kebabBtn = screen.getByLabelText(/playlist actions/i); + await fireEvent.click(kebabBtn); + const refreshItem = screen.getByRole('menuitem', { name: /refresh for you/i }); + await fireEvent.click(refreshItem); + await waitFor(() => expect(refreshForYou).toHaveBeenCalled()); + }); + + test('For-You play button does NOT call refreshForYou (uses existing snapshot)', async () => { const { refreshForYou, getPlaylist } = await import('$lib/api/playlists'); const { playQueue } = await import('$lib/player/store.svelte'); const forYouPlaylist: Playlist = { @@ -220,70 +238,50 @@ describe('PlaylistCard', () => { name: 'For You', track_count: 25 }; - // getPlaylist should return a detail with the NEW id - (getPlaylist as ReturnType).mockResolvedValueOnce({ - id: 'p-new', - user_id: 'u-self', - owner_username: 'me', - name: 'For You', - description: '', - is_public: false, - kind: 'system', - system_variant: 'for_you', - seed_artist_id: null, - cover_url: '', - track_count: 1, - duration_sec: 60, - created_at: '2026-01-01T00:00:00Z', - updated_at: '2026-01-01T00:00:00Z', - tracks: [ - { - position: 0, - track_id: 't-1', - album_id: 'al-1', - artist_id: 'ar-1', - title: 'Test Track', - artist_name: 'Test Artist', - album_title: 'Test Album', - duration_sec: 60, - stream_url: '/s/t-1', - added_at: '2026-01-01T00:00:00Z' - } - ] - } as PlaylistDetail); render(PlaylistCard, { props: { playlist: forYouPlaylist } }); const playButton = screen.getByLabelText(/Play For You/i); await fireEvent.click(playButton); await new Promise(resolve => setTimeout(resolve, 10)); - expect(refreshForYou).toHaveBeenCalled(); - expect(getPlaylist).toHaveBeenCalledWith('p-new'); + // Play uses the existing playlist snapshot — no refresh on press. + expect(refreshForYou).not.toHaveBeenCalled(); + expect(getPlaylist).toHaveBeenCalledWith('p-1'); expect(playQueue).toHaveBeenCalled(); }); - test('For-You play with empty-library response does not call playQueue', async () => { - const { refreshForYou } = await import('$lib/api/playlists'); + test('For-You play passes shuffle:true to playQueue', async () => { const { playQueue } = await import('$lib/player/store.svelte'); - (refreshForYou as ReturnType).mockResolvedValueOnce({ - playlist_id: null, - track_count: 0, - track_ids: [] - }); const forYouPlaylist: Playlist = { ...base, kind: 'system', system_variant: 'for_you', - name: 'For You', - track_count: 1 + name: 'For You' }; render(PlaylistCard, { props: { playlist: forYouPlaylist } }); const playButton = screen.getByLabelText(/Play For You/i); await fireEvent.click(playButton); await new Promise(resolve => setTimeout(resolve, 10)); - expect(refreshForYou).toHaveBeenCalled(); - expect(playQueue).not.toHaveBeenCalled(); + expect(playQueue).toHaveBeenCalledWith( + expect.any(Array), + 0, + expect.objectContaining({ shuffle: true }) + ); + }); + + test('user-playlist play passes shuffle:false to playQueue', async () => { + const { playQueue } = await import('$lib/player/store.svelte'); + render(PlaylistCard, { props: { playlist: base } }); + const playButton = screen.getByLabelText(/Play Saturday morning/i); + await fireEvent.click(playButton); + await new Promise(resolve => setTimeout(resolve, 10)); + + expect(playQueue).toHaveBeenCalledWith( + expect.any(Array), + 0, + expect.objectContaining({ shuffle: false }) + ); }); test('non-For-You play button does NOT call refreshForYou', async () => { diff --git a/web/src/lib/player/store.svelte.ts b/web/src/lib/player/store.svelte.ts index 6a0d9d82..8339916f 100644 --- a/web/src/lib/player/store.svelte.ts +++ b/web/src/lib/player/store.svelte.ts @@ -62,15 +62,34 @@ export function registerAudioEl(el: HTMLAudioElement | null): void { _audioEl = el; } -export function playQueue(tracks: TrackRef[], startIndex = 0): void { +export function playQueue( + tracks: TrackRef[], + startIndex = 0, + opts: { shuffle?: boolean } = {}, +): void { _radioSeedId = null; // M4c: non-radio enqueue clears the radio refresh state - _queue = tracks; - if (tracks.length === 0) { + if (opts.shuffle && tracks.length > 1) { + // Fisher-Yates over the whole list. startIndex is ignored — the + // caller is asking for "random play from this pool," so the first + // track should also be random, not the one at startIndex. + const arr = tracks.slice(); + for (let j = arr.length - 1; j > 0; j--) { + const r = Math.floor(_rng() * (j + 1)); + [arr[j], arr[r]] = [arr[r], arr[j]]; + } + _queue = arr; _index = 0; - _state = 'idle'; - } else { - _index = Math.max(0, Math.min(startIndex, tracks.length - 1)); + _shuffle = true; _state = 'loading'; + } else { + _queue = tracks; + if (tracks.length === 0) { + _index = 0; + _state = 'idle'; + } else { + _index = Math.max(0, Math.min(startIndex, tracks.length - 1)); + _state = 'loading'; + } } _position = 0; _duration = 0; From 7a0437087a06a8f9c813f0c7df20e704cf32881d Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 06:35:55 -0400 Subject: [PATCH 03/27] feat(flutter): refresh kebab on system playlist cards (#416 parity) Closes the Flutter half of Fable #416. Web got a generalized system-playlist refresh kebab in d12afda; this brings Flutter to parity instead of leaving the affordance web-only. - PlaylistsApi.refreshSystem(variant): POST /api/playlists/system/{for-you|discover}/refresh, maps the underscore model variant to the hyphenated route segment, returns the rotated playlist id. - PlaylistCard: top-right PopupMenuButton on system playlists with a context-labelled "Refresh For You" / "Refresh Discover" item. Calls refreshSystem, invalidates playlistsListProvider (which reconciles the rotated UUID + new tracks), snackbars the result. ScaffoldMessenger captured pre-await. - Tests: kebab present for system, absent for user playlists. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../lib/api/endpoints/playlists.dart | 16 ++++++ .../lib/playlists/widgets/playlist_card.dart | 51 +++++++++++++++++++ .../playlists/widgets/playlist_card_test.dart | 20 ++++++++ 3 files changed, 87 insertions(+) diff --git a/flutter_client/lib/api/endpoints/playlists.dart b/flutter_client/lib/api/endpoints/playlists.dart index 9d772dd8..4e841b0f 100644 --- a/flutter_client/lib/api/endpoints/playlists.dart +++ b/flutter_client/lib/api/endpoints/playlists.dart @@ -53,4 +53,20 @@ class PlaylistsApi { data: {'track_ids': trackIds}, ); } + + /// POST /api/playlists/system/{variant}/refresh. Synchronously + /// rebuilds the caller's system playlist for the given variant + /// ("for_you" | "discover") and returns the new playlist id, or + /// null when the library is empty so there's nothing to build. + /// + /// The variant uses underscores in the model (system_variant) but + /// the route segment is hyphenated ("for-you"), so map here. + Future refreshSystem(String variant) async { + final segment = variant == 'for_you' ? 'for-you' : variant; + final r = await _dio.post>( + '/api/playlists/system/$segment/refresh', + data: const {}, + ); + return (r.data ?? const {})['playlist_id'] as String?; + } } diff --git a/flutter_client/lib/playlists/widgets/playlist_card.dart b/flutter_client/lib/playlists/widgets/playlist_card.dart index 66566558..ccb25613 100644 --- a/flutter_client/lib/playlists/widgets/playlist_card.dart +++ b/flutter_client/lib/playlists/widgets/playlist_card.dart @@ -66,6 +66,31 @@ class PlaylistCard extends ConsumerWidget { onPressed: () => _playPlaylist(ref), ), ), + // System playlists get a refresh affordance so the + // user can force a fresh mix instead of waiting for + // the daily 03:00 rebuild. Mirrors the web kebab. + if (playlist.isSystem) + Positioned( + top: 2, + right: 2, + child: PopupMenuButton( + icon: Icon(Icons.more_vert, color: fs.parchment, size: 18), + tooltip: 'Playlist actions', + color: fs.iron, + onSelected: (_) => _refresh(context, ref), + itemBuilder: (_) => [ + PopupMenuItem( + value: 'refresh', + child: Row(children: [ + Icon(Icons.refresh, color: fs.parchment, size: 18), + const SizedBox(width: 8), + Text(_refreshLabel, + style: TextStyle(color: fs.parchment)), + ]), + ), + ], + ), + ), ], ), const SizedBox(height: 8), @@ -98,6 +123,32 @@ class PlaylistCard extends ConsumerWidget { ); } + String get _refreshLabel => switch (playlist.systemVariant) { + 'for_you' => 'Refresh For You', + 'discover' => 'Refresh Discover', + _ => 'Refresh', + }; + + /// Forces a server-side rebuild of this system playlist, then + /// invalidates the aggregate list so the rotated UUID + new tracks + /// land on the next read. ScaffoldMessenger is captured before the + /// await so we don't touch a stale BuildContext afterward. + Future _refresh(BuildContext context, WidgetRef ref) async { + final messenger = ScaffoldMessenger.of(context); + try { + final api = await ref.read(playlistsApiProvider.future); + await api.refreshSystem(playlist.systemVariant!); + ref.invalidate(playlistsListProvider); + messenger.showSnackBar( + SnackBar(content: Text('${playlist.name} refreshed')), + ); + } catch (e) { + messenger.showSnackBar( + SnackBar(content: Text("Couldn't refresh: $e")), + ); + } + } + /// Fetches the playlist via /api/playlists/{id}, materializes each /// PlaylistTrack into a TrackRef (filtering out unavailable rows /// whose `trackId` is null after a track-delete), and plays from diff --git a/flutter_client/test/playlists/widgets/playlist_card_test.dart b/flutter_client/test/playlists/widgets/playlist_card_test.dart index 2d04a589..228a9707 100644 --- a/flutter_client/test/playlists/widgets/playlist_card_test.dart +++ b/flutter_client/test/playlists/widgets/playlist_card_test.dart @@ -55,4 +55,24 @@ void main() { expect(find.text('For You'), findsOneWidget); expect(find.text('for you'), findsOneWidget); }); + + testWidgets('shows refresh kebab on system playlists', (tester) async { + await tester.pumpWidget(MaterialApp( + theme: buildThemeData(), + home: const Scaffold( + body: PlaylistCard(playlist: _forYou), + ), + )); + expect(find.byIcon(Icons.more_vert), findsOneWidget); + }); + + testWidgets('no refresh kebab on user playlists', (tester) async { + await tester.pumpWidget(MaterialApp( + theme: buildThemeData(), + home: const Scaffold( + body: PlaylistCard(playlist: _userPlaylist), + ), + )); + expect(find.byIcon(Icons.more_vert), findsNothing); + }); } From 45c72993f3ba123fcbe085e3437ef27660443826 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 06:55:37 -0400 Subject: [PATCH 04/27] feat(flutter): replace Download with Regenerate on system playlist detail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per operator decision: in the playlist detail header, system playlists (For You / Discover) now show a Regenerate button where user playlists keep Download. Offline-download is intentionally dropped for system playlists — operator chose the literal swap. - Regenerate calls PlaylistsApi.refreshSystem(variant), invalidates playlistsListProvider (home row tile rebinds to the rotated UUID), and pushReplacement's to /playlists/ so the open detail screen rebinds instead of 404-ing on the stale id. - Null id (empty library) and errors surface as snackbars. - User playlists are unchanged (Download + Play). The home-card kebab (#416, 7a04370) stays — web has refresh in both the detail view and the home tile, so this matches web parity. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../lib/playlists/playlist_detail_screen.dart | 75 ++++++++++++++----- 1 file changed, 57 insertions(+), 18 deletions(-) diff --git a/flutter_client/lib/playlists/playlist_detail_screen.dart b/flutter_client/lib/playlists/playlist_detail_screen.dart index 50c4d12b..957abd9d 100644 --- a/flutter_client/lib/playlists/playlist_detail_screen.dart +++ b/flutter_client/lib/playlists/playlist_detail_screen.dart @@ -263,24 +263,32 @@ class _Header extends ConsumerWidget { ), const Spacer(), if (playable.isNotEmpty) ...[ - OutlinedButton.icon( - key: const Key('download_playlist_button'), - onPressed: () { - final mgr = ref.read(audioCacheManagerProvider); - for (final t in playable) { - final id = t.trackId ?? ''; - if (id.isEmpty) continue; - // Fire-and-forget; downloads happen in background. - // ignore: unawaited_futures - mgr.pin(id, source: CacheSource.autoPlaylist); - } - ScaffoldMessenger.of(context).showSnackBar( - SnackBar(content: Text('Downloading ${playable.length} tracks…')), - ); - }, - icon: const Icon(Icons.download, size: 16), - label: const Text('Download'), - ), + if (p.isSystem) + OutlinedButton.icon( + key: const Key('regenerate_playlist_button'), + onPressed: () => _regenerate(context, ref), + icon: const Icon(Icons.refresh, size: 16), + label: const Text('Regenerate'), + ) + else + OutlinedButton.icon( + key: const Key('download_playlist_button'), + onPressed: () { + final mgr = ref.read(audioCacheManagerProvider); + for (final t in playable) { + final id = t.trackId ?? ''; + if (id.isEmpty) continue; + // Fire-and-forget; downloads happen in background. + // ignore: unawaited_futures + mgr.pin(id, source: CacheSource.autoPlaylist); + } + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Downloading ${playable.length} tracks…')), + ); + }, + icon: const Icon(Icons.download, size: 16), + label: const Text('Download'), + ), const SizedBox(width: 8), FilledButton.icon( onPressed: () { @@ -299,6 +307,37 @@ class _Header extends ConsumerWidget { ]), ); } + + /// Forces a server-side rebuild of this system playlist. The + /// rebuild rotates the playlist UUID, so the old detail route now + /// 404s — rebind by pushReplacement-ing to the new id. The + /// aggregate list is invalidated too so the home row's tile points + /// at the fresh UUID. ScaffoldMessenger + router captured before + /// the await so we don't touch a stale BuildContext after. + Future _regenerate(BuildContext context, WidgetRef ref) async { + final messenger = ScaffoldMessenger.of(context); + final router = GoRouter.of(context); + final p = detail.playlist; + try { + final api = await ref.read(playlistsApiProvider.future); + final newId = await api.refreshSystem(p.systemVariant!); + ref.invalidate(playlistsListProvider); + if (newId == null) { + messenger.showSnackBar(const SnackBar( + content: Text('Nothing to build yet — library is empty.'), + )); + return; + } + messenger.showSnackBar( + SnackBar(content: Text('${p.name} regenerated')), + ); + router.pushReplacement('/playlists/$newId'); + } catch (e) { + messenger.showSnackBar( + SnackBar(content: Text("Couldn't regenerate: $e")), + ); + } + } } class _PlaylistTrackRow extends ConsumerWidget { From 69569a5c2bf31f82d0745405e68b1f7bee6c62c0 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 07:32:55 -0400 Subject: [PATCH 05/27] feat(playlists): expand For You to 100 tracks (Discover parity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes Fable #414. forYouHeadN/forYouTailN go 12/13 → 50/50 so the For-You snapshot is 100 tracks, matching Discover. Motivated by the shuffle-on-play default that just shipped (#413): a 25-track shuffle pool repeats fast; 100 makes re-plays within a day feel varied. pickHeadAndTail already degrades gracefully when the candidate pool is too thin for a full head/tail split (returns top-N-by-score), mirroring how Discover returns <100 when its buckets are thin — no new edge-case handling needed. No build-path test asserts the For-You total; pickHeadAndTail unit tests pass their own head/tail values so they're unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/playlists/system.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/internal/playlists/system.go b/internal/playlists/system.go index 63c1f800..7bb03c20 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -151,14 +151,22 @@ var systemMixWeights = recommendation.ScoringWeights{ // forYouHeadN is the number of top-scored tracks that anchor the For-You // playlist; forYouTailN is the number sampled from positions 2*headN -// onward, daily-deterministic via tieBreakHash. The 12 + 13 split +// onward, daily-deterministic via tieBreakHash. The 50 + 50 split // (~50% from the tail) gives the user a substantially different mix // day-to-day while preserving a recognizable anchor of strong -// similarity matches. Songs-like-X keeps simple pickTopN (the +// similarity matches. +// +// Sized to 100 to match Discover so the shuffle-on-play default +// (system playlists shuffle when launched from a tile) has a deep +// enough pool that re-plays within a day feel varied. Libraries with +// a thin For-You candidate pool degrade gracefully: pickHeadAndTail +// returns top-N-by-score when the capped pool can't support a full +// head/tail split, exactly as Discover returns fewer than 100 when +// its buckets are thin. Songs-like-X keeps simple pickTopN (the // seed-artist context already frames the "you'll like this" promise). const ( - forYouHeadN = 12 - forYouTailN = 13 + forYouHeadN = 50 + forYouTailN = 50 ) // scoreAndSortCandidates scores every candidate with recommendation.Score From d2a0b7d780c45c60675f9f47add202040c91c5cd Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 07:40:17 -0400 Subject: [PATCH 06/27] =?UTF-8?q?feat(playlists):=20#415=20stage=201=20?= =?UTF-8?q?=E2=80=94=20rotation-state=20schema=20+=20play=20ingest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First of three stages for system-playlist sample-history dedup (Fable #415, server-side rotation per the operator's choice). Schema (migration 0027): - play_events.source (nullable text): which surface a play came from. 'for_you' / 'discover' feed rotation; NULL for library / user-playlist / radio / Subsonic. - system_playlist_rotation_state(user_id, playlist_kind, played_track_ids uuid[], rotation_started_at, updated_at): the per-(user,kind) set of already-heard tracks this rotation. Ingest: - New RecordPlayStartedWithSource on the writer; RecordPlayStarted is now a thin source="" wrapper so the frozen Subsonic shim is untouched (no signature ripple). - When source is a known system kind, the same txn appends the track to rotation state (AppendRotationPlayed keeps the array a set via the conflict CASE). - /api/events play_started accepts an optional "source". No serve-behavior change yet — Stage 2 makes shuffle prefer the unplayed tail + resets on exhaustion; Stage 3 wires the clients to send source and consume the rotation-aware order. Tests: rotation appends + dedupes for a system source; source-less play writes no rotation row. (Existing RecordPlayStarted tests are unchanged — same wrapper signature, identical behavior at source="".) Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/events.go | 10 +- internal/db/dbq/events.sql.go | 18 ++-- internal/db/dbq/models.go | 9 ++ .../db/dbq/system_playlist_rotation.sql.go | 91 +++++++++++++++++++ .../0027_system_playlist_rotation.down.sql | 5 + .../0027_system_playlist_rotation.up.sql | 26 ++++++ internal/db/queries/events.sql | 4 +- .../db/queries/system_playlist_rotation.sql | 36 ++++++++ internal/playevents/writer.go | 41 ++++++++- internal/playevents/writer_test.go | 49 ++++++++++ 10 files changed, 279 insertions(+), 10 deletions(-) create mode 100644 internal/db/dbq/system_playlist_rotation.sql.go create mode 100644 internal/db/migrations/0027_system_playlist_rotation.down.sql create mode 100644 internal/db/migrations/0027_system_playlist_rotation.up.sql create mode 100644 internal/db/queries/system_playlist_rotation.sql diff --git a/internal/api/events.go b/internal/api/events.go index 2e95ee36..6bdc898e 100644 --- a/internal/api/events.go +++ b/internal/api/events.go @@ -20,6 +20,10 @@ type eventRequest struct { PositionMs *int32 `json:"position_ms"` At *string `json:"at"` ClientID *string `json:"client_id"` + // Source tags which surface a play_started came from. "for_you" + // / "discover" feed the system-playlist rotation dedup (#415); + // absent / "" for library, user-playlist, radio, Subsonic. + Source *string `json:"source"` } type playStartedResponse struct { @@ -84,7 +88,11 @@ func (h *handlers) handleEventPlayStarted( writeErr(w, apierror.InternalMsg("lookup failed", err)) return } - res, err := h.events.RecordPlayStarted(r.Context(), user.ID, trackID, clientID, at) + source := "" + if req.Source != nil { + source = *req.Source + } + res, err := h.events.RecordPlayStartedWithSource(r.Context(), user.ID, trackID, clientID, source, at) if err != nil { h.logger.Error("api: events: play_started", "err", err) writeErr(w, apierror.InternalMsg("record failed", err)) diff --git a/internal/db/dbq/events.sql.go b/internal/db/dbq/events.sql.go index 83a04dee..59f08d04 100644 --- a/internal/db/dbq/events.sql.go +++ b/internal/db/dbq/events.sql.go @@ -54,7 +54,7 @@ func (q *Queries) GetMostRecentPlaySessionForUser(ctx context.Context, userID pg } const getOpenPlayEventForUser = `-- name: GetOpenPlayEventForUser :one -SELECT id, user_id, track_id, session_id, started_at, ended_at, duration_played_ms, completion_ratio, was_skipped, client_id, session_vector_at_play, scrobbled_at FROM play_events +SELECT id, user_id, track_id, session_id, started_at, ended_at, duration_played_ms, completion_ratio, was_skipped, client_id, session_vector_at_play, scrobbled_at, source FROM play_events WHERE user_id = $1 AND ended_at IS NULL ORDER BY started_at DESC LIMIT 1 @@ -78,12 +78,13 @@ func (q *Queries) GetOpenPlayEventForUser(ctx context.Context, userID pgtype.UUI &i.ClientID, &i.SessionVectorAtPlay, &i.ScrobbledAt, + &i.Source, ) return i, err } const getPlayEventByID = `-- name: GetPlayEventByID :one -SELECT id, user_id, track_id, session_id, started_at, ended_at, duration_played_ms, completion_ratio, was_skipped, client_id, session_vector_at_play, scrobbled_at FROM play_events WHERE id = $1 +SELECT id, user_id, track_id, session_id, started_at, ended_at, duration_played_ms, completion_ratio, was_skipped, client_id, session_vector_at_play, scrobbled_at, source FROM play_events WHERE id = $1 ` func (q *Queries) GetPlayEventByID(ctx context.Context, id pgtype.UUID) (PlayEvent, error) { @@ -102,15 +103,16 @@ func (q *Queries) GetPlayEventByID(ctx context.Context, id pgtype.UUID) (PlayEve &i.ClientID, &i.SessionVectorAtPlay, &i.ScrobbledAt, + &i.Source, ) return i, err } const insertPlayEvent = `-- name: InsertPlayEvent :one INSERT INTO play_events ( - user_id, track_id, session_id, started_at, client_id -) VALUES ($1, $2, $3, $4, $5) -RETURNING id, user_id, track_id, session_id, started_at, ended_at, duration_played_ms, completion_ratio, was_skipped, client_id, session_vector_at_play, scrobbled_at + user_id, track_id, session_id, started_at, client_id, source +) VALUES ($1, $2, $3, $4, $5, $6) +RETURNING id, user_id, track_id, session_id, started_at, ended_at, duration_played_ms, completion_ratio, was_skipped, client_id, session_vector_at_play, scrobbled_at, source ` type InsertPlayEventParams struct { @@ -119,6 +121,7 @@ type InsertPlayEventParams struct { SessionID pgtype.UUID StartedAt pgtype.Timestamptz ClientID *string + Source *string } func (q *Queries) InsertPlayEvent(ctx context.Context, arg InsertPlayEventParams) (PlayEvent, error) { @@ -128,6 +131,7 @@ func (q *Queries) InsertPlayEvent(ctx context.Context, arg InsertPlayEventParams arg.SessionID, arg.StartedAt, arg.ClientID, + arg.Source, ) var i PlayEvent err := row.Scan( @@ -143,6 +147,7 @@ func (q *Queries) InsertPlayEvent(ctx context.Context, arg InsertPlayEventParams &i.ClientID, &i.SessionVectorAtPlay, &i.ScrobbledAt, + &i.Source, ) return i, err } @@ -286,7 +291,7 @@ SET ended_at = $2, completion_ratio = $4, was_skipped = $5 WHERE id = $1 -RETURNING id, user_id, track_id, session_id, started_at, ended_at, duration_played_ms, completion_ratio, was_skipped, client_id, session_vector_at_play, scrobbled_at +RETURNING id, user_id, track_id, session_id, started_at, ended_at, duration_played_ms, completion_ratio, was_skipped, client_id, session_vector_at_play, scrobbled_at, source ` type UpdatePlayEventEndedParams struct { @@ -322,6 +327,7 @@ func (q *Queries) UpdatePlayEventEnded(ctx context.Context, arg UpdatePlayEventE &i.ClientID, &i.SessionVectorAtPlay, &i.ScrobbledAt, + &i.Source, ) return i, err } diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index f99894a5..3b772075 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -369,6 +369,7 @@ type PlayEvent struct { ClientID *string SessionVectorAtPlay []byte ScrobbledAt pgtype.Timestamptz + Source *string } type PlaySession struct { @@ -475,6 +476,14 @@ type SmtpConfig struct { UpdatedAt pgtype.Timestamptz } +type SystemPlaylistRotationState struct { + UserID pgtype.UUID + PlaylistKind string + PlayedTrackIds []pgtype.UUID + RotationStartedAt pgtype.Timestamptz + UpdatedAt pgtype.Timestamptz +} + type SystemPlaylistRun struct { UserID pgtype.UUID LastRunAt pgtype.Timestamptz diff --git a/internal/db/dbq/system_playlist_rotation.sql.go b/internal/db/dbq/system_playlist_rotation.sql.go new file mode 100644 index 00000000..6aeff689 --- /dev/null +++ b/internal/db/dbq/system_playlist_rotation.sql.go @@ -0,0 +1,91 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: system_playlist_rotation.sql + +package dbq + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const appendRotationPlayed = `-- name: AppendRotationPlayed :exec + +INSERT INTO system_playlist_rotation_state ( + user_id, playlist_kind, played_track_ids, rotation_started_at, updated_at +) VALUES ( + $1, $2, + array_append('{}'::uuid[], $3::uuid), now(), now() +) +ON CONFLICT (user_id, playlist_kind) DO UPDATE +SET played_track_ids = + CASE WHEN $3::uuid = ANY(system_playlist_rotation_state.played_track_ids) + THEN system_playlist_rotation_state.played_track_ids + ELSE array_append(system_playlist_rotation_state.played_track_ids, $3::uuid) + END, + updated_at = now() +` + +type AppendRotationPlayedParams struct { + UserID pgtype.UUID + PlaylistKind string + TrackID pgtype.UUID +} + +// Rotation state for system-playlist sample-history dedup (#415). +// One row per (user, playlist_kind). played_track_ids accumulates +// tracks heard from that playlist in the current rotation; shuffle +// prefers ids NOT in this set, then resets when the snapshot is +// exhausted. +// Records that the given track was played from the (user, kind) +// system playlist. Creates the row on first play; on conflict +// appends only if not already present so the array stays a set. +func (q *Queries) AppendRotationPlayed(ctx context.Context, arg AppendRotationPlayedParams) error { + _, err := q.db.Exec(ctx, appendRotationPlayed, arg.UserID, arg.PlaylistKind, arg.TrackID) + return err +} + +const getRotationState = `-- name: GetRotationState :one +SELECT user_id, playlist_kind, played_track_ids, rotation_started_at, updated_at FROM system_playlist_rotation_state +WHERE user_id = $1 AND playlist_kind = $2 +` + +type GetRotationStateParams struct { + UserID pgtype.UUID + PlaylistKind string +} + +func (q *Queries) GetRotationState(ctx context.Context, arg GetRotationStateParams) (SystemPlaylistRotationState, error) { + row := q.db.QueryRow(ctx, getRotationState, arg.UserID, arg.PlaylistKind) + var i SystemPlaylistRotationState + err := row.Scan( + &i.UserID, + &i.PlaylistKind, + &i.PlayedTrackIds, + &i.RotationStartedAt, + &i.UpdatedAt, + ) + return i, err +} + +const resetRotationState = `-- name: ResetRotationState :exec +UPDATE system_playlist_rotation_state +SET played_track_ids = '{}', + rotation_started_at = now(), + updated_at = now() +WHERE user_id = $1 AND playlist_kind = $2 +` + +type ResetRotationStateParams struct { + UserID pgtype.UUID + PlaylistKind string +} + +// Clears the played set and advances the rotation window. Called +// when the playlist snapshot has been fully sampled (Stage 2). +func (q *Queries) ResetRotationState(ctx context.Context, arg ResetRotationStateParams) error { + _, err := q.db.Exec(ctx, resetRotationState, arg.UserID, arg.PlaylistKind) + return err +} diff --git a/internal/db/migrations/0027_system_playlist_rotation.down.sql b/internal/db/migrations/0027_system_playlist_rotation.down.sql new file mode 100644 index 00000000..c52a8f5f --- /dev/null +++ b/internal/db/migrations/0027_system_playlist_rotation.down.sql @@ -0,0 +1,5 @@ +-- 0027_system_playlist_rotation.down.sql +DROP TABLE IF EXISTS system_playlist_rotation_state; + +ALTER TABLE play_events + DROP COLUMN IF EXISTS source; diff --git a/internal/db/migrations/0027_system_playlist_rotation.up.sql b/internal/db/migrations/0027_system_playlist_rotation.up.sql new file mode 100644 index 00000000..3314a4ea --- /dev/null +++ b/internal/db/migrations/0027_system_playlist_rotation.up.sql @@ -0,0 +1,26 @@ +-- 0027_system_playlist_rotation.up.sql — sample-history dedup for +-- system playlists (Fable #415). Two parts: +-- +-- 1. play_events.source: which system playlist a play originated +-- from ('for_you' | 'discover' | future kinds), or NULL for any +-- play not launched from a system-playlist surface (library, +-- user playlist, radio, Subsonic). Lets the ingest path know a +-- play should count against that playlist's rotation. +-- +-- 2. system_playlist_rotation_state: per (user, playlist_kind) set +-- of track ids already played in the current rotation window. +-- Shuffle-on-play prefers the unplayed tail; when the snapshot +-- is exhausted the set resets and rotation_started_at advances. +-- Centralized server-side so phone / web / watch share one +-- rotation, mirroring how radio dedup already lives server-side. +ALTER TABLE play_events + ADD COLUMN source text; + +CREATE TABLE system_playlist_rotation_state ( + user_id uuid NOT NULL REFERENCES users(id) ON DELETE CASCADE, + playlist_kind text NOT NULL, + played_track_ids uuid[] NOT NULL DEFAULT '{}', + rotation_started_at timestamptz NOT NULL DEFAULT now(), + updated_at timestamptz NOT NULL DEFAULT now(), + PRIMARY KEY (user_id, playlist_kind) +); diff --git a/internal/db/queries/events.sql b/internal/db/queries/events.sql index 60e42f4d..db0d4829 100644 --- a/internal/db/queries/events.sql +++ b/internal/db/queries/events.sql @@ -25,8 +25,8 @@ LIMIT 1; -- name: InsertPlayEvent :one INSERT INTO play_events ( - user_id, track_id, session_id, started_at, client_id -) VALUES ($1, $2, $3, $4, $5) + user_id, track_id, session_id, started_at, client_id, source +) VALUES ($1, $2, $3, $4, $5, $6) RETURNING *; -- name: UpdatePlayEventEnded :one diff --git a/internal/db/queries/system_playlist_rotation.sql b/internal/db/queries/system_playlist_rotation.sql new file mode 100644 index 00000000..3f95050e --- /dev/null +++ b/internal/db/queries/system_playlist_rotation.sql @@ -0,0 +1,36 @@ +-- Rotation state for system-playlist sample-history dedup (#415). +-- One row per (user, playlist_kind). played_track_ids accumulates +-- tracks heard from that playlist in the current rotation; shuffle +-- prefers ids NOT in this set, then resets when the snapshot is +-- exhausted. + +-- name: AppendRotationPlayed :exec +-- Records that the given track was played from the (user, kind) +-- system playlist. Creates the row on first play; on conflict +-- appends only if not already present so the array stays a set. +INSERT INTO system_playlist_rotation_state ( + user_id, playlist_kind, played_track_ids, rotation_started_at, updated_at +) VALUES ( + sqlc.arg(user_id), sqlc.arg(playlist_kind), + array_append('{}'::uuid[], sqlc.arg(track_id)::uuid), now(), now() +) +ON CONFLICT (user_id, playlist_kind) DO UPDATE +SET played_track_ids = + CASE WHEN sqlc.arg(track_id)::uuid = ANY(system_playlist_rotation_state.played_track_ids) + THEN system_playlist_rotation_state.played_track_ids + ELSE array_append(system_playlist_rotation_state.played_track_ids, sqlc.arg(track_id)::uuid) + END, + updated_at = now(); + +-- name: GetRotationState :one +SELECT * FROM system_playlist_rotation_state +WHERE user_id = $1 AND playlist_kind = $2; + +-- name: ResetRotationState :exec +-- Clears the played set and advances the rotation window. Called +-- when the playlist snapshot has been fully sampled (Stage 2). +UPDATE system_playlist_rotation_state +SET played_track_ids = '{}', + rotation_started_at = now(), + updated_at = now() +WHERE user_id = $1 AND playlist_kind = $2; diff --git a/internal/playevents/writer.go b/internal/playevents/writer.go index 00bff645..a218f16a 100644 --- a/internal/playevents/writer.go +++ b/internal/playevents/writer.go @@ -57,12 +57,36 @@ type StartedResult struct { // RecordPlayStarted: in one transaction, auto-close any prior open row for // the user, FindOrCreate the play_session, insert the new play_event, -// return its id and session id. +// return its id and session id. Source-agnostic — kept for the Subsonic +// shim (frozen API), which never originates from a system playlist. func (w *Writer) RecordPlayStarted( ctx context.Context, userID, trackID pgtype.UUID, clientID string, at time.Time, +) (StartedResult, error) { + return w.RecordPlayStartedWithSource(ctx, userID, trackID, clientID, "", at) +} + +// systemPlaylistSources are the play_events.source values that count +// against a system playlist's rotation (Fable #415). Add future +// system-playlist kinds here as they ship (deep_cuts, rediscover, …). +var systemPlaylistSources = map[string]bool{ + "for_you": true, + "discover": true, +} + +// RecordPlayStartedWithSource is RecordPlayStarted plus a `source` +// tag identifying which surface the play came from. When source is a +// known system-playlist kind the track is appended to that user's +// rotation set so shuffle-on-play can prefer the unplayed tail on a +// re-play within the day. Empty source → behaves exactly like the +// pre-#415 path (NULL column, no rotation write). +func (w *Writer) RecordPlayStartedWithSource( + ctx context.Context, + userID, trackID pgtype.UUID, + clientID, source string, + at time.Time, ) (StartedResult, error) { var out StartedResult err := pgx.BeginFunc(ctx, w.pool, func(tx pgx.Tx) error { @@ -78,12 +102,17 @@ func (w *Writer) RecordPlayStarted( if clientID != "" { clientIDPtr = &clientID } + var sourcePtr *string + if source != "" { + sourcePtr = &source + } ev, err := q.InsertPlayEvent(ctx, dbq.InsertPlayEventParams{ UserID: userID, TrackID: trackID, SessionID: sessionID, StartedAt: pgtype.Timestamptz{Time: at, Valid: true}, ClientID: clientIDPtr, + Source: sourcePtr, }) if err != nil { return err @@ -95,6 +124,16 @@ func (w *Writer) RecordPlayStarted( if err := w.captureSessionVector(ctx, q, ev.ID, sessionID, at); err != nil { return err } + + if systemPlaylistSources[source] { + if err := q.AppendRotationPlayed(ctx, dbq.AppendRotationPlayedParams{ + UserID: userID, + PlaylistKind: source, + TrackID: trackID, + }); err != nil { + return err + } + } return nil }) return out, err diff --git a/internal/playevents/writer_test.go b/internal/playevents/writer_test.go index 6f9d8233..bfb30dc7 100644 --- a/internal/playevents/writer_test.go +++ b/internal/playevents/writer_test.go @@ -372,3 +372,52 @@ func TestRecordPlayEnded_SubThreshold_NoEnqueue(t *testing.T) { t.Errorf("expected 0 queue rows for sub-threshold play, got %d", n) } } + +func TestRecordPlayStartedWithSource_AppendsRotation(t *testing.T) { + f := newFixture(t, 200_000) + now := time.Now().UTC() + if _, err := f.w.RecordPlayStartedWithSource( + context.Background(), f.user, f.track, "c", "for_you", now, + ); err != nil { + t.Fatalf("RecordPlayStartedWithSource: %v", err) + } + st, err := f.q.GetRotationState(context.Background(), dbq.GetRotationStateParams{ + UserID: f.user, PlaylistKind: "for_you", + }) + if err != nil { + t.Fatalf("GetRotationState: %v", err) + } + if len(st.PlayedTrackIds) != 1 || st.PlayedTrackIds[0] != f.track { + t.Errorf("played_track_ids = %v, want [%v]", st.PlayedTrackIds, f.track) + } + + // Re-playing the same track stays a set (no duplicate append). + if _, err := f.w.RecordPlayStartedWithSource( + context.Background(), f.user, f.track, "c", "for_you", now.Add(time.Minute), + ); err != nil { + t.Fatalf("RecordPlayStartedWithSource 2: %v", err) + } + st, err = f.q.GetRotationState(context.Background(), dbq.GetRotationStateParams{ + UserID: f.user, PlaylistKind: "for_you", + }) + if err != nil { + t.Fatalf("GetRotationState 2: %v", err) + } + if len(st.PlayedTrackIds) != 1 { + t.Errorf("played_track_ids should dedupe, got %v", st.PlayedTrackIds) + } +} + +func TestRecordPlayStarted_NoRotationWithoutSource(t *testing.T) { + f := newFixture(t, 200_000) + now := time.Now().UTC() + if _, err := f.w.RecordPlayStarted(context.Background(), f.user, f.track, "c", now); err != nil { + t.Fatalf("RecordPlayStarted: %v", err) + } + _, err := f.q.GetRotationState(context.Background(), dbq.GetRotationStateParams{ + UserID: f.user, PlaylistKind: "for_you", + }) + if err == nil { + t.Errorf("expected no rotation row for a source-less play") + } +} From e43281d1d0b31418fad0aca60147558df4804ddc Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 07:47:57 -0400 Subject: [PATCH 07/27] =?UTF-8?q?feat(playlists):=20#415=20stage=202=20?= =?UTF-8?q?=E2=80=94=20rotation-aware=20shuffle=20endpoint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/playlists/system/{discover,for-you}/shuffle returns the caller's system playlist with tracks ordered: unplayed-this-rotation first (shuffled), then already-heard (shuffled). When the whole snapshot has been heard, ResetRotationState fires and the full list reshuffles fresh. Option A (operator's choice): a separate, intentionally-uncached endpoint. The cached GET /api/playlists/{id} detail path stays pure for "open to view"; this varies per play. Same JSON shape as the detail GET so Stage 3 clients reuse track parsing with no new model. Two explicit static routes per variant mirror the refresh handlers and avoid chi static-vs-param ambiguity under /playlists/system/. Empty/absent snapshot → 200 with empty track list (nothing to play, not an error). Rotation reset failure is non-fatal — still returns a playable reshuffled list. No client wiring yet — Stage 3 makes web + Flutter call this on the play/tile gesture and send `source` on play_started. Handler-level test deferred to Stage 3 (needs the full service+pool harness; the end-to-end path is exercised there). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/api.go | 2 + internal/api/playlists_system_shuffle.go | 128 +++++++++++++++++++++++ 2 files changed, 130 insertions(+) create mode 100644 internal/api/playlists_system_shuffle.go diff --git a/internal/api/api.go b/internal/api/api.go index 6e039e5d..3cbf2dbf 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -174,6 +174,8 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Get("/playlists/{id}/cover", h.handleGetPlaylistCover) authed.Post("/playlists/system/discover/refresh", h.handleDiscoverRefresh) authed.Post("/playlists/system/for-you/refresh", h.handleForYouRefresh) + authed.Get("/playlists/system/discover/shuffle", h.handleDiscoverShuffle) + authed.Get("/playlists/system/for-you/shuffle", h.handleForYouShuffle) }) }) } diff --git a/internal/api/playlists_system_shuffle.go b/internal/api/playlists_system_shuffle.go new file mode 100644 index 00000000..e531f3f0 --- /dev/null +++ b/internal/api/playlists_system_shuffle.go @@ -0,0 +1,128 @@ +package api + +import ( + "errors" + "math/rand" + "net/http" + + "github.com/jackc/pgx/v5" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/playlists" +) + +// Stage 2 of #415. GET /api/playlists/system/{variant}/shuffle returns +// the caller's system playlist with tracks in rotation-aware play +// order: tracks not yet heard this rotation first (shuffled), then +// already-heard tracks (shuffled). When the whole snapshot has been +// heard the rotation resets and the full list reshuffles. +// +// Same JSON shape as GET /api/playlists/{id} so clients reuse their +// existing track parsing. Intentionally a separate, uncached endpoint +// (Option A): the cached playlist-detail GET stays pure for the +// "open the playlist to look at it" path; this one varies per play. +// +// Two explicit static routes (one per variant) mirror the refresh +// handlers and sidestep chi static-vs-param ambiguity under +// /playlists/system/. Future kinds add their own thin route. + +func (h *handlers) handleForYouShuffle(w http.ResponseWriter, r *http.Request) { + h.serveSystemPlaylistShuffle(w, r, "for_you") +} + +func (h *handlers) handleDiscoverShuffle(w http.ResponseWriter, r *http.Request) { + h.serveSystemPlaylistShuffle(w, r, "discover") +} + +func (h *handlers) serveSystemPlaylistShuffle(w http.ResponseWriter, r *http.Request, variant string) { + user, ok := requireUser(w, r) + if !ok { + return + } + q := dbq.New(h.pool) + + v := variant + pl, err := q.GetSystemPlaylistByVariantForUser(r.Context(), + dbq.GetSystemPlaylistByVariantForUserParams{ + UserID: user.ID, + SystemVariant: &v, + }) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + // No snapshot yet (empty library / not built). Empty list, + // not an error — the client just has nothing to play. + writeJSON(w, http.StatusOK, playlistDetailView{ + Tracks: []playlistTrackView{}, + }) + return + } + h.logger.Error("system shuffle: lookup failed", "variant", variant, "err", err) + writeErr(w, apierror.InternalMsg("lookup failed", err)) + return + } + + detail, err := h.playlists.Get(r.Context(), user.ID, pl.ID) + if err != nil { + h.writePlaylistErr(w, err, "system shuffle") + return + } + + // Played set for the current rotation. ErrNoRows = nothing played + // from this playlist yet → empty set, everything counts unplayed. + played := map[[16]byte]bool{} + st, rerr := q.GetRotationState(r.Context(), dbq.GetRotationStateParams{ + UserID: user.ID, PlaylistKind: variant, + }) + if rerr != nil && !errors.Is(rerr, pgx.ErrNoRows) { + h.logger.Error("system shuffle: rotation lookup failed", "variant", variant, "err", rerr) + writeErr(w, apierror.InternalMsg("rotation lookup failed", rerr)) + return + } + if rerr == nil { + for _, id := range st.PlayedTrackIds { + if id.Valid { + played[id.Bytes] = true + } + } + } + + // Partition playable tracks. Rows whose upstream track was deleted + // (TrackID nil) are dropped — nothing to enqueue. + unplayed := make([]playlists.PlaylistTrack, 0, len(detail.Tracks)) + heard := make([]playlists.PlaylistTrack, 0) + for _, t := range detail.Tracks { + if t.TrackID == nil || !t.TrackID.Valid { + continue + } + if played[t.TrackID.Bytes] { + heard = append(heard, t) + } else { + unplayed = append(unplayed, t) + } + } + + // Whole snapshot heard → reset the rotation and treat everything + // as fresh. Reset failure is non-fatal: we still return a + // playable reshuffled list; the next play just re-resets. + if len(unplayed) == 0 && len(heard) > 0 { + if err := q.ResetRotationState(r.Context(), dbq.ResetRotationStateParams{ + UserID: user.ID, PlaylistKind: variant, + }); err != nil { + h.logger.Warn("system shuffle: rotation reset failed", + "variant", variant, "err", err) + } + unplayed = heard + heard = nil + } + + rand.Shuffle(len(unplayed), func(i, j int) { + unplayed[i], unplayed[j] = unplayed[j], unplayed[i] + }) + rand.Shuffle(len(heard), func(i, j int) { + heard[i], heard[j] = heard[j], heard[i] + }) + + detail.Tracks = append(unplayed, heard...) + writeJSON(w, http.StatusOK, playlistDetailToView(detail)) +} From 179519689b79e3010cf33aaea8d1a8ef3d838626 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 07:57:00 -0400 Subject: [PATCH 08/27] =?UTF-8?q?feat(web):=20#415=20stage=203=20(web)=20?= =?UTF-8?q?=E2=80=94=20rotation-aware=20system=20playlist=20play?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Web half of Stage 3. System-playlist tile play now: - calls the new GET /api/playlists/system/{variant}/shuffle endpoint (rotation-aware order from the server) instead of getPlaylist; plays the returned order AS-IS — no client Fisher-Yates, since the server already ordered it. Supersedes #413's client shuffle for system playlists specifically; user playlists keep getPlaylist + stored order. - tags the queue with the system variant. The player store carries _queueSource; the events dispatcher includes `source` on play_started so the server advances that playlist's rotation. User playlists are unchanged (getPlaylist, plain playQueue, no source). Tests updated: For-You play hits systemShuffle (not getPlaylist/refresh) and passes source:for_you; user play uses getPlaylist + plain playQueue with no source. Flutter half is blocked — the Flutter client has no play-event reporting at all (no /api/events POST, no scrobble), so there's no play_started to attach `source` to. Surfacing that as a separate decision rather than silently scope-exploding #415. Co-Authored-By: Claude Opus 4.7 (1M context) --- web/src/lib/api/playlists.ts | 12 +++++ web/src/lib/components/PlaylistCard.svelte | 27 ++++++---- web/src/lib/components/PlaylistCard.test.ts | 55 ++++++++++++++++----- web/src/lib/player/events.svelte.ts | 6 ++- web/src/lib/player/store.svelte.ts | 14 +++++- 5 files changed, 91 insertions(+), 23 deletions(-) diff --git a/web/src/lib/api/playlists.ts b/web/src/lib/api/playlists.ts index 2b442068..e1dfa192 100644 --- a/web/src/lib/api/playlists.ts +++ b/web/src/lib/api/playlists.ts @@ -19,6 +19,18 @@ export async function getPlaylist(id: string): Promise { return api.get(`/api/playlists/${encodeURIComponent(id)}`); } +// #415 stage 2/3: rotation-aware play order for a system playlist. +// Same shape as getPlaylist but tracks are server-ordered (unplayed +// this rotation first, then heard; resets when exhausted). The model +// variant uses underscores; the route segment is hyphenated. +// Intentionally uncached — it varies per play, unlike getPlaylist. +export async function systemShuffle(variant: string): Promise { + const seg = variant === 'for_you' ? 'for-you' : variant; + return api.get( + `/api/playlists/system/${encodeURIComponent(seg)}/shuffle`, + ); +} + export type CreatePlaylistInput = { name: string; description?: string; diff --git a/web/src/lib/components/PlaylistCard.svelte b/web/src/lib/components/PlaylistCard.svelte index 78c64bb7..92f41875 100644 --- a/web/src/lib/components/PlaylistCard.svelte +++ b/web/src/lib/components/PlaylistCard.svelte @@ -3,7 +3,7 @@ import { useQueryClient } from '@tanstack/svelte-query'; import type { Playlist, PlaylistTrack, TrackRef } from '$lib/api/types'; import { user } from '$lib/auth/store.svelte'; - import { getPlaylist, refreshDiscover, refreshForYou } from '$lib/api/playlists'; + import { getPlaylist, systemShuffle, refreshDiscover, refreshForYou } from '$lib/api/playlists'; import { playlistTrackToRef } from '$lib/playlists/playlistTrackToRef'; import { errCode } from '$lib/api/errors'; import { qk } from '$lib/api/queries'; @@ -45,14 +45,23 @@ if (starting) return; starting = true; try { - const detail = await getPlaylist(playlist.id); - const refs = toTrackRefs(detail.tracks); - if (refs.length > 0) { - // System playlists default to shuffle so replaying within a - // day feels different. User playlists keep stored order. - // Explicit refresh moved to the kebab menu — playing no - // longer rebuilds the snapshot. - playQueue(refs, 0, { shuffle: playlist.system_variant != null }); + const variant = playlist.system_variant; + if (variant != null) { + // #415: server returns the rotation-aware order (unplayed + // this rotation first). Play it as-is — no client shuffle — + // and tag the queue so play_started carries `source` and + // the server advances the rotation. + const detail = await systemShuffle(variant); + const refs = toTrackRefs(detail.tracks); + if (refs.length > 0) { + playQueue(refs, 0, { source: variant }); + } + } else { + const detail = await getPlaylist(playlist.id); + const refs = toTrackRefs(detail.tracks); + if (refs.length > 0) { + playQueue(refs, 0); + } } } finally { starting = false; diff --git a/web/src/lib/components/PlaylistCard.test.ts b/web/src/lib/components/PlaylistCard.test.ts index 688f86d9..5eac8386 100644 --- a/web/src/lib/components/PlaylistCard.test.ts +++ b/web/src/lib/components/PlaylistCard.test.ts @@ -38,6 +38,36 @@ vi.mock('$lib/api/playlists', () => ({ } ] } as PlaylistDetail), + systemShuffle: vi.fn().mockResolvedValue({ + id: 'p-sys', + user_id: 'u-self', + owner_username: 'me', + name: 'For You', + description: '', + is_public: false, + kind: 'system', + system_variant: 'for_you', + seed_artist_id: null, + cover_url: '', + track_count: 1, + duration_sec: 60, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + tracks: [ + { + position: 0, + track_id: 't-9', + album_id: 'al-1', + artist_id: 'ar-1', + title: 'Rotation Track', + artist_name: 'Test Artist', + album_title: 'Test Album', + duration_sec: 60, + stream_url: '/s/t-9', + added_at: '2026-01-01T00:00:00Z' + } + ] + } as PlaylistDetail), refreshDiscover: vi.fn().mockResolvedValue({ playlist_id: 'p-1', track_count: 5 }), refreshForYou: vi.fn().mockResolvedValue({ playlist_id: 'p-new', track_count: 25, track_ids: ['t-1'] }) })); @@ -228,8 +258,8 @@ describe('PlaylistCard', () => { await waitFor(() => expect(refreshForYou).toHaveBeenCalled()); }); - test('For-You play button does NOT call refreshForYou (uses existing snapshot)', async () => { - const { refreshForYou, getPlaylist } = await import('$lib/api/playlists'); + test('For-You play calls systemShuffle, not getPlaylist or refreshForYou', async () => { + const { refreshForYou, getPlaylist, systemShuffle } = await import('$lib/api/playlists'); const { playQueue } = await import('$lib/player/store.svelte'); const forYouPlaylist: Playlist = { ...base, @@ -244,13 +274,15 @@ describe('PlaylistCard', () => { await fireEvent.click(playButton); await new Promise(resolve => setTimeout(resolve, 10)); - // Play uses the existing playlist snapshot — no refresh on press. + // #415: system play hits the rotation-aware shuffle endpoint, + // not the cached detail GET, and never refreshes on press. + expect(systemShuffle).toHaveBeenCalledWith('for_you'); + expect(getPlaylist).not.toHaveBeenCalled(); expect(refreshForYou).not.toHaveBeenCalled(); - expect(getPlaylist).toHaveBeenCalledWith('p-1'); expect(playQueue).toHaveBeenCalled(); }); - test('For-You play passes shuffle:true to playQueue', async () => { + test('For-You play passes source:for_you to playQueue (no client shuffle)', async () => { const { playQueue } = await import('$lib/player/store.svelte'); const forYouPlaylist: Playlist = { ...base, @@ -266,22 +298,21 @@ describe('PlaylistCard', () => { expect(playQueue).toHaveBeenCalledWith( expect.any(Array), 0, - expect.objectContaining({ shuffle: true }) + expect.objectContaining({ source: 'for_you' }) ); }); - test('user-playlist play passes shuffle:false to playQueue', async () => { + test('user-playlist play uses getPlaylist + plain playQueue (no source)', async () => { + const { getPlaylist, systemShuffle } = await import('$lib/api/playlists'); const { playQueue } = await import('$lib/player/store.svelte'); render(PlaylistCard, { props: { playlist: base } }); const playButton = screen.getByLabelText(/Play Saturday morning/i); await fireEvent.click(playButton); await new Promise(resolve => setTimeout(resolve, 10)); - expect(playQueue).toHaveBeenCalledWith( - expect.any(Array), - 0, - expect.objectContaining({ shuffle: false }) - ); + expect(systemShuffle).not.toHaveBeenCalled(); + expect(getPlaylist).toHaveBeenCalledWith('p-1'); + expect(playQueue).toHaveBeenCalledWith(expect.any(Array), 0); }); test('non-For-You play button does NOT call refreshForYou', async () => { diff --git a/web/src/lib/player/events.svelte.ts b/web/src/lib/player/events.svelte.ts index 60d82236..ab383c51 100644 --- a/web/src/lib/player/events.svelte.ts +++ b/web/src/lib/player/events.svelte.ts @@ -92,7 +92,11 @@ export function useEventsDispatcher(): void { const res = await api.post('/api/events', { type: 'play_started', track_id: trackId, - client_id: clientId + client_id: clientId, + // #415: tag the play with the system playlist it came from + // (null for library / user-playlist / radio) so the server + // can advance that playlist's rotation. + source: player.queueSource ?? undefined }); // The user may have moved on by the time the response arrives. Only // adopt the id if we're still on the same track and still playing. diff --git a/web/src/lib/player/store.svelte.ts b/web/src/lib/player/store.svelte.ts index 8339916f..156d2b49 100644 --- a/web/src/lib/player/store.svelte.ts +++ b/web/src/lib/player/store.svelte.ts @@ -41,6 +41,13 @@ let _pendingRestorePosition: number | null = null; let _radioSeedId = $state(null); let _radioRefreshInFlight = false; +// #415: which system playlist (if any) this queue was seeded from. +// 'for_you' | 'discover' | null. Read by the events dispatcher so +// play_started carries `source` and the play counts against that +// playlist's rotation. Re-set on every playQueue (a fresh non-system +// queue clears it). +let _queueSource = $state(null); + let _audioEl: HTMLAudioElement | null = null; export const player = { @@ -55,6 +62,7 @@ export const player = { get shuffle() { return _shuffle; }, get repeat() { return _repeat; }, get error() { return _error; }, + get queueSource() { return _queueSource; }, get queueDrawerOpen() { return _queueDrawerOpen; } }; @@ -65,9 +73,13 @@ export function registerAudioEl(el: HTMLAudioElement | null): void { export function playQueue( tracks: TrackRef[], startIndex = 0, - opts: { shuffle?: boolean } = {}, + opts: { shuffle?: boolean; source?: string | null } = {}, ): void { _radioSeedId = null; // M4c: non-radio enqueue clears the radio refresh state + // #415: a fresh queue resets the system-playlist source. Set only + // when seeded from a system playlist (server already returned the + // rotation-aware order, so no client shuffle in that path). + _queueSource = opts.source ?? null; if (opts.shuffle && tracks.length > 1) { // Fisher-Yates over the whole list. startIndex is ignored — the // caller is asking for "random play from this pool," so the first From 3054e8702b961efc15d71896b40eb3b428dc0093 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 08:15:32 -0400 Subject: [PATCH 09/27] =?UTF-8?q?feat(flutter):=20#415=20stage=203=20?= =?UTF-8?q?=E2=80=94=20play-events=20reporter=20+=20rotation=20wiring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Flutter client previously reported NO plays — mobile listening never reached play_events, so history, recommendation scoring, ListenBrainz scrobbles, and #415 rotation all missed mobile entirely. Operator chose to close that gap properly as part of Stage 3. New: - EventsApi (api/endpoints/events.dart): play_started/ended/skipped. - PlayEventsReporter (player/play_events_reporter.dart): state machine over (track id, playing) mirroring the web dispatcher. Persists an opaque client_id in secure storage. Deliberate divergence from web: a track change inside a queue is classified ended-vs-skipped by whether the prior track reached ~its duration (3s tolerance), instead of web's blanket "track change = skip" which would mark every naturally-finished in-queue track a skip and dilute recommendation skip-ratios — the exact failure mode that motivated doing this properly. Fail-safe: no-ops when there's no audio handler (tests / no-audio env). App-lifecycle paused/ detached closes an open row as a best-effort skip (web pagehide parity). Wired in app.dart postFrame. - PlaylistsApi.systemShuffle(variant): GET the rotation-aware order. Wiring: - audio_handler: _queueSource carried through setQueueFromTracks (source param); preserved across internal skipToQueueItem rebuild. - player_provider.playTracks: source param → setQueueFromTracks. - PlaylistCard: system playlists fetch systemShuffle and play as-is tagged with source (no client shuffle — server already ordered). - playlist_detail_screen: header Play + per-track tap tag source for system playlists so rotation advances from any entry point. Known/flagged separately: the web dispatcher likely has the same false-skip-on-advance issue; not fixed here to keep #415 scoped and clients' wire behavior comparable. Co-Authored-By: Claude Opus 4.7 (1M context) --- flutter_client/lib/api/endpoints/events.dart | 65 ++++++ .../lib/api/endpoints/playlists.dart | 13 ++ flutter_client/lib/app.dart | 6 + flutter_client/lib/player/audio_handler.dart | 20 +- .../lib/player/play_events_reporter.dart | 198 ++++++++++++++++++ .../lib/player/player_provider.dart | 9 +- .../lib/playlists/playlist_detail_screen.dart | 8 +- .../lib/playlists/widgets/playlist_card.dart | 17 +- 8 files changed, 325 insertions(+), 11 deletions(-) create mode 100644 flutter_client/lib/api/endpoints/events.dart create mode 100644 flutter_client/lib/player/play_events_reporter.dart diff --git a/flutter_client/lib/api/endpoints/events.dart b/flutter_client/lib/api/endpoints/events.dart new file mode 100644 index 00000000..a20d9376 --- /dev/null +++ b/flutter_client/lib/api/endpoints/events.dart @@ -0,0 +1,65 @@ +import 'package:dio/dio.dart'; + +/// Thin client for POST /api/events — the play-event lifecycle the +/// server uses for history, recommendation scoring, ListenBrainz +/// scrobbles, and (since #415) system-playlist rotation. +/// +/// Mirrors the web events dispatcher's three calls. Best-effort by +/// contract: callers swallow errors — a missed event is acceptable +/// per the server spec's v1 stance, and the server's +/// auto-close-prior-open keeps history sane even if an ended/skipped +/// is lost. +class EventsApi { + EventsApi(this._dio); + final Dio _dio; + + /// POST play_started. Returns the server's play_event_id (used to + /// close the row later), or null if the call failed / response was + /// malformed. `source` tags the originating system playlist + /// ('for_you' | 'discover') so the server advances that rotation; + /// omit for library / user-playlist / radio plays. + Future playStarted({ + required String trackId, + required String clientId, + String? source, + }) async { + final r = await _dio.post>( + '/api/events', + data: { + 'type': 'play_started', + 'track_id': trackId, + 'client_id': clientId, + if (source != null && source.isNotEmpty) 'source': source, + }, + ); + return (r.data ?? const {})['play_event_id'] as String?; + } + + Future playEnded({ + required String playEventId, + required int durationPlayedMs, + }) async { + await _dio.post( + '/api/events', + data: { + 'type': 'play_ended', + 'play_event_id': playEventId, + 'duration_played_ms': durationPlayedMs, + }, + ); + } + + Future playSkipped({ + required String playEventId, + required int positionMs, + }) async { + await _dio.post( + '/api/events', + data: { + 'type': 'play_skipped', + 'play_event_id': playEventId, + 'position_ms': positionMs, + }, + ); + } +} diff --git a/flutter_client/lib/api/endpoints/playlists.dart b/flutter_client/lib/api/endpoints/playlists.dart index 4e841b0f..92f6d49b 100644 --- a/flutter_client/lib/api/endpoints/playlists.dart +++ b/flutter_client/lib/api/endpoints/playlists.dart @@ -45,6 +45,19 @@ class PlaylistsApi { return PlaylistDetail.fromJson(r.data ?? const {}); } + /// GET /api/playlists/system/{variant}/shuffle (#415 stage 2/3). + /// Same shape as get() but tracks are server-ordered rotation-aware + /// (unplayed-this-rotation first; resets when exhausted). Model + /// variant uses underscores; the route segment is hyphenated. + /// Intentionally uncached — varies per play. + Future systemShuffle(String variant) async { + final seg = variant == 'for_you' ? 'for-you' : variant; + final r = await _dio.get>( + '/api/playlists/system/$seg/shuffle', + ); + return PlaylistDetail.fromJson(r.data ?? const {}); + } + /// POST /api/playlists/{id}/tracks. Owner only; server returns the /// playlist detail with the new rows. Future appendTracks(String playlistId, List trackIds) async { diff --git a/flutter_client/lib/app.dart b/flutter_client/lib/app.dart index 0c39d806..d4a1150e 100644 --- a/flutter_client/lib/app.dart +++ b/flutter_client/lib/app.dart @@ -6,6 +6,7 @@ import 'cache/metadata_prefetcher.dart'; import 'cache/mutation_queue.dart'; import 'cache/prefetcher.dart'; import 'cache/sync_controller.dart'; +import 'player/play_events_reporter.dart'; import 'shared/live_events_dispatcher.dart'; import 'shared/routing.dart'; import 'theme/theme_data.dart'; @@ -55,6 +56,11 @@ class _MinstrelAppState extends ConsumerState { // enqueue on REST failure so user intent persists across // network loss instead of getting rolled back. ref.read(mutationReplayerProvider); + // Play-events reporter (#415): the Flutter client otherwise + // reports no plays at all — this feeds history, recommendation + // scoring, scrobbles, and system-playlist rotation, and is the + // path that carries the `source` tag for #415. + ref.read(playEventsReporterProvider); }); } diff --git a/flutter_client/lib/player/audio_handler.dart b/flutter_client/lib/player/audio_handler.dart index 6217ae7d..b8156f08 100644 --- a/flutter_client/lib/player/audio_handler.dart +++ b/flutter_client/lib/player/audio_handler.dart @@ -82,6 +82,15 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl /// past the fill front needs to rebuild from the stored tracks. List _lastTracks = const []; + /// #415: which system playlist this queue was seeded from + /// ('for_you' | 'discover'), or null for library / user-playlist / + /// radio. The play-events reporter reads this so play_started + /// carries `source` and the server advances that rotation. A fresh + /// setQueueFromTracks from a non-system surface clears it; internal + /// rebuilds (skipToQueueItem) preserve it. + String? _queueSource; + String? get queueSource => _queueSource; + /// Volume stream for UI subscribers. Mirrors the just_audio player's /// volume directly; set via setVolume(double). Stream get volumeStream => _player.volumeStream; @@ -109,8 +118,13 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl if (audioCacheManager != null) _audioCacheManager = audioCacheManager; } - Future setQueueFromTracks(List tracks, {int initialIndex = 0}) async { + Future setQueueFromTracks( + List tracks, { + int initialIndex = 0, + String? source, + }) async { if (tracks.isEmpty) return; + _queueSource = source; final clampedInitial = initialIndex.clamp(0, tracks.length - 1); // Bump the generation FIRST. Any in-flight _fillRemainingSources @@ -190,7 +204,9 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl @override Future skipToQueueItem(int index) async { if (index < 0 || index >= _lastTracks.length) return; - await setQueueFromTracks(_lastTracks, initialIndex: index); + // Preserve the system-playlist source across an internal rebuild + // — a queue-item skip is still playing from the same playlist. + await setQueueFromTracks(_lastTracks, initialIndex: index, source: _queueSource); await play(); } diff --git a/flutter_client/lib/player/play_events_reporter.dart b/flutter_client/lib/player/play_events_reporter.dart new file mode 100644 index 00000000..3c3fb9dd --- /dev/null +++ b/flutter_client/lib/player/play_events_reporter.dart @@ -0,0 +1,198 @@ +// Flutter play-event lifecycle reporter (#415 stage 3). +// +// The Flutter client previously reported NO plays — listening on +// mobile never reached the server's play_events, so history, +// recommendation scoring, ListenBrainz scrobbles, and (since #415) +// system-playlist rotation all missed mobile activity entirely. This +// closes that gap and is the path that carries the #415 `source` tag. +// +// State machine over (current track id, playing). Mirrors the web +// events dispatcher, with one deliberate divergence: when a track +// changes inside a queue we classify ended-vs-skipped by whether the +// prior track reached (near) its duration, instead of the web +// dispatcher's blanket "track change = skip". Blanket-skip would mark +// every naturally-finished in-queue track as a skip and dilute the +// recommendation skip-ratio — the exact failure mode that motivated +// doing this properly. (The web dispatcher likely has the same +// false-skip issue; flagged separately, not fixed here.) + +import 'dart:async'; +import 'dart:math'; + +import 'package:audio_service/audio_service.dart'; +import 'package:flutter/widgets.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; + +import '../api/endpoints/events.dart'; +import '../auth/auth_provider.dart' show secureStorageProvider; +import '../library/library_providers.dart' show dioProvider; +import 'audio_handler.dart' show MinstrelAudioHandler; +import 'player_provider.dart' show audioHandlerProvider; + +const _clientIdKey = 'play_events_client_id'; + +/// Tolerance for "the track basically finished": within 3s of the +/// known duration counts as a natural completion, not a skip. +const _completionToleranceMs = 3000; + +class PlayEventsReporter with WidgetsBindingObserver { + PlayEventsReporter(this._ref); + final Ref _ref; + + final _subs = >[]; + bool _disposed = false; + + String? _clientId; + EventsApi? _api; + + String? _openPlayEventId; + String? _openTrackId; + String? _prevTrackId; + int _lastPositionMs = 0; + int _openDurationMs = 0; + + Future start() async { + final MinstrelAudioHandler handler; + try { + // audioHandlerProvider throws until main() overrides it (real + // app always does). In tests / no-audio environments there's + // nothing to report — fail safe and stay inert rather than + // surfacing an unhandled async error. + handler = _ref.read(audioHandlerProvider); + _clientId = await _resolveClientId(); + final dio = await _ref.read(dioProvider.future); + _api = EventsApi(dio); + } catch (_) { + return; + } + if (_disposed) return; + WidgetsBinding.instance.addObserver(this); + + _subs.add(handler.positionStream.listen((p) { + _lastPositionMs = p.inMilliseconds; + // Keep the open track's duration current while it's the one + // playing, so on a track change we still know how long the + // (now-prior) track was. + final mi = handler.mediaItem.value; + if (mi != null && mi.id == _openTrackId && mi.duration != null) { + _openDurationMs = mi.duration!.inMilliseconds; + } + })); + _subs.add(handler.mediaItem.listen((_) => _evaluate(handler))); + _subs.add(handler.playbackState.listen((_) => _evaluate(handler))); + } + + void _evaluate(MinstrelAudioHandler handler) { + if (_disposed) return; + final mi = handler.mediaItem.value; + final st = handler.playbackState.value; + final tid = mi?.id; + final playing = st.playing; + final completed = st.processingState == AudioProcessingState.completed; + + // Track changed with an open row → close the prior row. Natural + // completion (reached ~duration) ends it; otherwise it's a skip. + if (tid != _prevTrackId && _openPlayEventId != null) { + final id = _openPlayEventId!; + final pos = _lastPositionMs; + _openPlayEventId = null; + _openTrackId = null; + final finished = _openDurationMs > 0 && + pos >= _openDurationMs - _completionToleranceMs; + if (finished) { + _api?.playEnded(playEventId: id, durationPlayedMs: pos) + .catchError((_) {}); + } else { + _api?.playSkipped(playEventId: id, positionMs: pos) + .catchError((_) {}); + } + } + + // Entered playing for a new track → open a row. + if (tid != null && playing && _openTrackId != tid) { + _startNew(handler, tid); + } + + // Whole-queue natural end (just_audio only emits `completed` at + // the end of the sequence, not between items) → end the open row. + if (completed && _openPlayEventId != null) { + final id = _openPlayEventId!; + final dur = _lastPositionMs; + _openPlayEventId = null; + _openTrackId = null; + _api?.playEnded(playEventId: id, durationPlayedMs: dur) + .catchError((_) {}); + } + + _prevTrackId = tid; + } + + Future _startNew(MinstrelAudioHandler handler, String trackId) async { + final api = _api; + final cid = _clientId; + if (api == null || cid == null) return; + try { + final id = await api.playStarted( + trackId: trackId, + clientId: cid, + source: handler.queueSource, + ); + // The user may have moved on by the time the response lands — + // only adopt the id if we're still on the same track. + if (id != null && handler.mediaItem.value?.id == trackId) { + _openPlayEventId = id; + _openTrackId = trackId; + final d = handler.mediaItem.value?.duration; + _openDurationMs = d?.inMilliseconds ?? 0; + } + } catch (_) { + // Best-effort; a missed event is acceptable per spec v1. + } + } + + @override + void didChangeAppLifecycleState(AppLifecycleState state) { + // App backgrounded / killed with a live row: best-effort skip + // close so the row doesn't dangle. Mirrors web's pagehide beacon. + if (state == AppLifecycleState.paused || + state == AppLifecycleState.detached) { + final id = _openPlayEventId; + if (id != null) { + _api + ?.playSkipped(playEventId: id, positionMs: _lastPositionMs) + .catchError((_) {}); + } + } + } + + Future _resolveClientId() async { + final storage = _ref.read(secureStorageProvider); + final existing = await storage.read(key: _clientIdKey); + if (existing != null && existing.isNotEmpty) return existing; + final rnd = Random.secure(); + final bytes = List.generate(16, (_) => rnd.nextInt(256)); + final id = + bytes.map((b) => b.toRadixString(16).padLeft(2, '0')).join(); + await storage.write(key: _clientIdKey, value: id); + return id; + } + + void dispose() { + _disposed = true; + WidgetsBinding.instance.removeObserver(this); + for (final s in _subs) { + s.cancel(); + } + _subs.clear(); + } +} + +/// Read once at app start (app.dart postFrame) to activate reporting. +/// Disposed via ref.onDispose when the scope tears down. +final playEventsReporterProvider = Provider((ref) { + final r = PlayEventsReporter(ref); + ref.onDispose(r.dispose); + // ignore: unawaited_futures + r.start(); + return r; +}); diff --git a/flutter_client/lib/player/player_provider.dart b/flutter_client/lib/player/player_provider.dart index cc14d645..c1134aba 100644 --- a/flutter_client/lib/player/player_provider.dart +++ b/flutter_client/lib/player/player_provider.dart @@ -71,10 +71,13 @@ class PlayerActions { List tracks, { int initialIndex = 0, bool shuffle = false, + String? source, }) async { // shuffle=true means "play this pool randomly, starting at a - // random track." Used for system playlists so the first track of - // a re-play within the day is different from the prior one. + // random track" (client-side; used for non-system surfaces that + // want shuffle). System playlists instead fetch a server-ordered + // list and pass source — no client shuffle, the order is already + // rotation-aware (#415). var startAt = initialIndex; if (shuffle && tracks.length > 1) { startAt = Random().nextInt(tracks.length); @@ -91,7 +94,7 @@ class PlayerActions { audioCacheManager: audioCache, likeBridge: _buildLikeBridge(), ); - await h.setQueueFromTracks(tracks, initialIndex: startAt); + await h.setQueueFromTracks(tracks, initialIndex: startAt, source: source); if (shuffle) { await h.setShuffleMode(AudioServiceShuffleMode.all); } diff --git a/flutter_client/lib/playlists/playlist_detail_screen.dart b/flutter_client/lib/playlists/playlist_detail_screen.dart index 957abd9d..7f0f59d8 100644 --- a/flutter_client/lib/playlists/playlist_detail_screen.dart +++ b/flutter_client/lib/playlists/playlist_detail_screen.dart @@ -212,6 +212,9 @@ class _Body extends ConsumerWidget { ref.read(playerActionsProvider).playTracks( playableRefs, initialIndex: startIdx >= 0 ? startIdx : 0, + source: detail.playlist.isSystem + ? detail.playlist.systemVariant + : null, ); // Keep liveTrack referenced to avoid an unused-variable // warning while we leave hooks for menu wiring later. @@ -293,7 +296,10 @@ class _Header extends ConsumerWidget { FilledButton.icon( onPressed: () { final refs = playable.map(_toTrackRef).toList(growable: false); - ref.read(playerActionsProvider).playTracks(refs); + ref.read(playerActionsProvider).playTracks( + refs, + source: p.isSystem ? p.systemVariant : null, + ); }, icon: const Icon(Icons.play_arrow), label: const Text('Play'), diff --git a/flutter_client/lib/playlists/widgets/playlist_card.dart b/flutter_client/lib/playlists/widgets/playlist_card.dart index ccb25613..0026834b 100644 --- a/flutter_client/lib/playlists/widgets/playlist_card.dart +++ b/flutter_client/lib/playlists/widgets/playlist_card.dart @@ -155,7 +155,13 @@ class PlaylistCard extends ConsumerWidget { /// index 0. Mirrors the web PlaylistCard's onPlayClick. Future _playPlaylist(WidgetRef ref) async { final api = await ref.read(playlistsApiProvider.future); - final detail = await api.get(playlist.id); + // System playlists: fetch the server's rotation-aware order + // (#415) and play it as-is, tagged with the source so the + // play-events reporter advances that playlist's rotation. User + // playlists keep stored order, untagged. + final detail = playlist.isSystem + ? await api.systemShuffle(playlist.systemVariant!) + : await api.get(playlist.id); final refs = []; for (final t in detail.tracks) { if (t.trackId == null) continue; @@ -171,13 +177,14 @@ class PlaylistCard extends ConsumerWidget { )); } if (refs.isEmpty) return; - // System playlists default to shuffle so replaying within a day - // doesn't deliver the identical experience. User playlists keep - // stored order. + // System playlists already arrive in rotation-aware order from + // the server (#415) — play as-is, tagged with the variant so the + // reporter advances rotation. No client shuffle. User playlists + // play in stored order, untagged. await ref.read(playerActionsProvider).playTracks( refs, initialIndex: 0, - shuffle: playlist.isSystem, + source: playlist.isSystem ? playlist.systemVariant : null, ); } } From 8652d7124e8b76e49ee720fb61e1e0399d45cf63 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 10:45:11 -0400 Subject: [PATCH 10/27] fix(web): #426 classify in-queue track end as play_ended, not skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The events dispatcher closed every prior open row as play_skipped on any track-id change. Server-side RecordPlaySkipped force-sets was_skipped=true regardless of completion, so a queue played start to finish reported tracks 1..N-1 as skips — inflating recommendation skip-ratios (-skipRatio*SkipPenalty) and degrading For You / Discover quality for web listeners. Now: on track change, close the prior row as play_ended if that track reached ~its duration (3s tolerance, matching the Flutter PlayEventsReporter), else play_skipped at the real last position. Race fix: the store synchronously resets position/duration to 0 on track change, so reading lastPositionMs at change-time would see 0 and misclassify. Track per-open-row state (openReachedEnd, openLastPositionMs, openDurationMs) updated ONLY while the open track is current — a track change can't clobber them before the close branch runs. Brings web wire behavior back in line with Flutter. Test added: auto-advance after reaching duration → play_ended, never skipped; existing mid-track-skip and pause-at-end tests still hold. Co-Authored-By: Claude Opus 4.7 (1M context) --- web/src/lib/player/events.svelte.test.ts | 29 +++++++++++ web/src/lib/player/events.svelte.ts | 61 +++++++++++++++++++----- 2 files changed, 79 insertions(+), 11 deletions(-) diff --git a/web/src/lib/player/events.svelte.test.ts b/web/src/lib/player/events.svelte.test.ts index 74e324f8..d7ba8cee 100644 --- a/web/src/lib/player/events.svelte.test.ts +++ b/web/src/lib/player/events.svelte.test.ts @@ -92,6 +92,35 @@ describe('useEventsDispatcher', () => { cleanup(); }); + test('auto-advance after reaching duration POSTs play_ended, not skip (#426)', async () => { + const cleanup = $effect.root(() => useEventsDispatcher()); + + playQueue([track('1', 200), track('2', 200)]); + flushSync(); + reportDuration(200); + reportStateFromAudio('playing'); + flushSync(); + await Promise.resolve(); + await Promise.resolve(); + // Track 1 plays through to its end... + reportTimeUpdate(200); + flushSync(); + // ...then the queue advances to track 2 (track id changes, store + // resets position/duration to 0 synchronously). + skipNext(); + flushSync(); + await Promise.resolve(); + + const closesForPe1 = (api.post as ReturnType).mock.calls + .map((c) => c[1] as { type: string; play_event_id?: string }) + .filter((b) => b.play_event_id === 'pe-1'); + // The finished track must close as ended, never skipped. + expect(closesForPe1.some((b) => b.type === 'play_ended')).toBe(true); + expect(closesForPe1.some((b) => b.type === 'play_skipped')).toBe(false); + + cleanup(); + }); + test('user-initiated skip mid-track POSTs play_skipped', async () => { const cleanup = $effect.root(() => useEventsDispatcher()); diff --git a/web/src/lib/player/events.svelte.ts b/web/src/lib/player/events.svelte.ts index ab383c51..44f1d148 100644 --- a/web/src/lib/player/events.svelte.ts +++ b/web/src/lib/player/events.svelte.ts @@ -9,21 +9,49 @@ import type { PlayStartedResponse, EventOkResponse } from '$lib/api/types'; // The dispatcher is a small state machine over (current track id, state). // Per spec data-flow: // - First transition into 'playing' for a new track → POST play_started. -// - Track id changes while a play is open → POST play_skipped on the prior. +// - Track id changes while a play is open → close the prior row: +// play_ended if it reached ~its duration, else play_skipped (#426). +// Without this every auto-advanced in-queue track was reported as a +// skip, force-flagging was_skipped=true server-side and poisoning +// the recommendation skip-ratio. Mirrors the Flutter reporter. // - Natural completion (state moves from 'playing' to 'paused' AND -// position >= duration > 0) → POST play_ended on the open row. +// position >= duration > 0) → POST play_ended on the open row +// (covers the last track / explicit pause-at-end). // - pagehide with a live row → navigator.sendBeacon a play_skipped. + +// Within this much of the known duration counts as "finished", not a +// skip. Matches the Flutter PlayEventsReporter tolerance. +const COMPLETION_TOLERANCE_MS = 3000; + export function useEventsDispatcher(): void { let openPlayEventId: string | null = null; let openTrackId: string | null = null; + // lastPositionMs tracks the *current* track's position (used by the + // pagehide beacon). openLastPositionMs / openReachedEnd track the + // *open row's* track specifically and are updated ONLY while that + // track is current — so a track change (which synchronously resets + // the store's position to 0 and duration to 0) can't clobber them + // before the close branch reads them. This is the #426 race fix. let lastPositionMs = 0; + let openLastPositionMs = 0; + let openDurationMs = 0; + let openReachedEnd = false; let prevTrackId: string | null = null; let prevState: string | null = null; const clientId = getOrCreateClientId(); - // Track the latest position for use by close events. Cheap; no API call. $effect(() => { - lastPositionMs = Math.round(player.position * 1000); + const posMs = Math.round(player.position * 1000); + lastPositionMs = posMs; + if (openTrackId && player.current?.id === openTrackId) { + openLastPositionMs = posMs; + if (player.duration > 0) { + openDurationMs = Math.round(player.duration * 1000); + if (posMs >= openDurationMs - COMPLETION_TOLERANCE_MS) { + openReachedEnd = true; + } + } + } }); // Main state machine. Reacts only to (current track id, state) transitions. @@ -32,17 +60,25 @@ export function useEventsDispatcher(): void { const s = player.state; const tid = t?.id ?? null; - // Track changed: close any prior open play as a skip. + // Track changed: close the prior open row. If that track reached + // ~its duration it finished naturally (auto-advance) → play_ended + // with its full duration; otherwise the user moved on early → + // play_skipped at the last position it actually reached (#426). if (tid !== prevTrackId && openPlayEventId) { const id = openPlayEventId; - const pos = lastPositionMs; + const finished = openReachedEnd; + const endedMs = openDurationMs > 0 ? openDurationMs : openLastPositionMs; + const skipMs = openLastPositionMs; openPlayEventId = null; openTrackId = null; - api.post('/api/events', { - type: 'play_skipped', - play_event_id: id, - position_ms: pos - }).catch(() => {}); + openDurationMs = 0; + openLastPositionMs = 0; + openReachedEnd = false; + api.post('/api/events', + finished + ? { type: 'play_ended', play_event_id: id, duration_played_ms: endedMs } + : { type: 'play_skipped', play_event_id: id, position_ms: skipMs } + ).catch(() => {}); } // Entered playing for a new track (no open row yet for this track). @@ -62,6 +98,9 @@ export function useEventsDispatcher(): void { const dur = lastPositionMs; openPlayEventId = null; openTrackId = null; + openDurationMs = 0; + openLastPositionMs = 0; + openReachedEnd = false; api.post('/api/events', { type: 'play_ended', play_event_id: id, From 47aa17885087b24d91ce1f3b6d96577ebe53305e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 11:14:21 -0400 Subject: [PATCH 11/27] =?UTF-8?q?feat(playevents):=20#426B=20server=20?= =?UTF-8?q?=E2=80=94=20RecordOfflinePlay=20+=20/api/events=20play=5Fofflin?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server half of the offline-replay capture. New writer path RecordOfflinePlay: writes a complete start+end play in one txn from a caller-supplied `at` + duration_played_ms, applying the spec §6 skip rule (same AND-of-thresholds as RecordPlayEnded) and threading `source` so #415 rotation advances for system-playlist plays just like the live path. Generalizes RecordSyntheticCompletedPlay (which hard-codes full completion). duration clamped to [0, track len]. New /api/events type "play_offline" → handleEventPlayOffline: validates track + duration, reuses the existing req.At parse so the play lands on the original timeline, not replay time. Subsonic shim + live 3-call lifecycle untouched. Flutter half next: EventsApi.playOffline, a play.offline MutationQueue kind, and PlayEventsReporter capturing the completed play + enqueuing it when the live calls have no server id / fail. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/events.go | 44 +++++++++++++ internal/playevents/writer.go | 113 ++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+) diff --git a/internal/api/events.go b/internal/api/events.go index 6bdc898e..c7e49f16 100644 --- a/internal/api/events.go +++ b/internal/api/events.go @@ -65,6 +65,8 @@ func (h *handlers) handleEvents(w http.ResponseWriter, r *http.Request) { h.handleEventPlayEnded(w, r, user, req, at) case "play_skipped": h.handleEventPlaySkipped(w, r, user, req, at) + case "play_offline": + h.handleEventPlayOffline(w, r, user, req, at, clientID) default: writeErr(w, apierror.BadRequest("bad_request", "unknown event type")) } @@ -104,6 +106,48 @@ func (h *handlers) handleEventPlayStarted( }) } +// handleEventPlayOffline records a complete play that happened with +// no/flaky connectivity, replayed from the Flutter offline mutation +// queue (#426 part B). `at` is the original (client) play-start time +// — parsed by handleEvents from req.At — so history lands on the +// real timeline, not replay time. duration_played_ms drives the skip +// rule; source advances #415 rotation for system-playlist plays. +func (h *handlers) handleEventPlayOffline( + w http.ResponseWriter, r *http.Request, + user dbq.User, req eventRequest, at time.Time, clientID string, +) { + trackID, ok := parseUUID(req.TrackID) + if !ok { + writeErr(w, apierror.BadRequest("bad_request", "invalid track_id")) + return + } + if req.DurationPlayedMs == nil || *req.DurationPlayedMs < 0 { + writeErr(w, apierror.BadRequest("bad_request", "duration_played_ms required and must be >= 0")) + return + } + if _, err := dbq.New(h.pool).GetTrackByID(r.Context(), trackID); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + writeErr(w, &apierror.Error{Status: 404, Code: "not_found", Message: "track not found"}) + return + } + h.logger.Error("api: events: offline lookup track", "err", err) + writeErr(w, apierror.InternalMsg("lookup failed", err)) + return + } + source := "" + if req.Source != nil { + source = *req.Source + } + if err := h.events.RecordOfflinePlay( + r.Context(), user.ID, trackID, clientID, source, at, *req.DurationPlayedMs, + ); err != nil { + h.logger.Error("api: events: play_offline", "err", err) + writeErr(w, apierror.InternalMsg("record failed", err)) + return + } + writeJSON(w, http.StatusOK, okResponse{OK: true}) +} + func (h *handlers) handleEventPlayEnded( w http.ResponseWriter, r *http.Request, user dbq.User, req eventRequest, at time.Time, diff --git a/internal/playevents/writer.go b/internal/playevents/writer.go index a218f16a..f523a3b4 100644 --- a/internal/playevents/writer.go +++ b/internal/playevents/writer.go @@ -306,6 +306,119 @@ func (w *Writer) RecordSyntheticCompletedPlay( return nil } +// RecordOfflinePlay writes a complete start+end play in one call from +// a caller-supplied timestamp + duration. This is the replay path for +// the Flutter offline mutation queue (#426 part B): a play that +// happened with no/flaky connectivity is captured locally as a +// completed unit and replayed here once back online, preserving the +// original `at`. +// +// Generalizes RecordSyntheticCompletedPlay: instead of assuming full +// completion it applies the spec §6 skip rule from the real +// durationPlayed vs track length (same AND-of-thresholds as +// RecordPlayEnded), and threads `source` so #415 rotation advances +// for system-playlist plays just like the live path. +// +// durationPlayedMs is clamped to [0, track duration] to absorb +// client clock / position drift. +func (w *Writer) RecordOfflinePlay( + ctx context.Context, + userID, trackID pgtype.UUID, + clientID, source string, + at time.Time, + durationPlayedMs int32, +) error { + var playEventID pgtype.UUID + if err := pgx.BeginFunc(ctx, w.pool, func(tx pgx.Tx) error { + q := dbq.New(tx) + track, err := q.GetTrackByID(ctx, trackID) + if err != nil { + return err + } + if err := w.autoClosePriorOpen(ctx, q, userID, at); err != nil { + return err + } + sessionID, err := playsessions.FindOrCreate(ctx, q, userID, at, clientID, w.sessionTimeout) + if err != nil { + return err + } + var clientIDPtr *string + if clientID != "" { + clientIDPtr = &clientID + } + var sourcePtr *string + if source != "" { + sourcePtr = &source + } + ev, err := q.InsertPlayEvent(ctx, dbq.InsertPlayEventParams{ + UserID: userID, + TrackID: trackID, + SessionID: sessionID, + StartedAt: pgtype.Timestamptz{Time: at, Valid: true}, + ClientID: clientIDPtr, + Source: sourcePtr, + }) + if err != nil { + return err + } + playEventID = ev.ID + if err := w.captureSessionVector(ctx, q, ev.ID, sessionID, at); err != nil { + return err + } + + played := durationPlayedMs + if played < 0 { + played = 0 + } + if track.DurationMs > 0 && played > track.DurationMs { + played = track.DurationMs + } + ratio := 0.0 + if track.DurationMs > 0 { + ratio = float64(played) / float64(track.DurationMs) + } + isSkip := ratio < w.skipMaxCompletionRatio && played < w.skipMaxDurationPlayedMs + ratioPtr := ratio + durPtr := played + if _, err := q.UpdatePlayEventEnded(ctx, dbq.UpdatePlayEventEndedParams{ + ID: ev.ID, + EndedAt: pgtype.Timestamptz{Time: at.Add(time.Duration(played) * time.Millisecond), Valid: true}, + DurationPlayedMs: &durPtr, + CompletionRatio: &ratioPtr, + WasSkipped: isSkip, + }); err != nil { + return err + } + if isSkip { + if _, err := q.InsertSkipEvent(ctx, dbq.InsertSkipEventParams{ + UserID: userID, + TrackID: trackID, + SessionID: sessionID, + SkippedAt: pgtype.Timestamptz{Time: at.Add(time.Duration(played) * time.Millisecond), Valid: true}, + PositionMs: played, + }); err != nil { + return err + } + } + if systemPlaylistSources[source] { + if err := q.AppendRotationPlayed(ctx, dbq.AppendRotationPlayedParams{ + UserID: userID, + PlaylistKind: source, + TrackID: trackID, + }); err != nil { + return err + } + } + return nil + }); err != nil { + return err + } + if err := scrobble.MaybeEnqueue(ctx, dbq.New(w.pool), playEventID); err != nil { + w.logger.Warn("playevents: scrobble enqueue failed", "play_event_id", playEventID, "err", err) + } + return nil +} + // captureSessionVector queries the user's prior plays in the given session // (before `at`), builds the session vector, and UPDATEs the just-inserted // play_event with it. Runs inside the caller's transaction (q is the From a571282031856e51686931a0dbc33b84d12024d4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 11:19:11 -0400 Subject: [PATCH 12/27] =?UTF-8?q?feat(flutter):=20#426B=20client=20?= =?UTF-8?q?=E2=80=94=20offline=20play=20capture=20via=20mutation=20queue?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flutter half of offline-replay capture. Play events no longer fire-and-forget: the reporter now tracks each play as a completed unit (track, original start time, source, duration reached) independently of connectivity. - EventsApi.playOffline: single timestamp-preserving call → the new /api/events play_offline (47aa178). - MutationQueue: new play.offline kind + handler (EventsApi). - PlayEventsReporter rework: - _beginTrack captures start context + fires live play_started; the server id is adopted only if it lands while still on-track. - position progress gated on the tracked track id so a track change can't clobber the finishing track's last values. - _closeCurrent: if a server id registered, attempt the live ended/skipped and fall back to the offline queue on failure; if no id (offline start) enqueue the completed play directly. The server applies the canonical skip rule, so the offline payload only carries duration. - app paused/detached closes durably via the queue (survives a process kill; a teardown POST would not). Result: listening to cached tracks fully offline now records history / recs / scrobble / #415 rotation once back online, with the original timestamps. Web stays best-effort by standing occasional-use scope. Co-Authored-By: Claude Opus 4.7 (1M context) --- flutter_client/lib/api/endpoints/events.dart | 26 +++ flutter_client/lib/cache/mutation_queue.dart | 15 ++ .../lib/player/play_events_reporter.dart | 183 +++++++++++++----- 3 files changed, 172 insertions(+), 52 deletions(-) diff --git a/flutter_client/lib/api/endpoints/events.dart b/flutter_client/lib/api/endpoints/events.dart index a20d9376..c5ab0fc0 100644 --- a/flutter_client/lib/api/endpoints/events.dart +++ b/flutter_client/lib/api/endpoints/events.dart @@ -62,4 +62,30 @@ class EventsApi { }, ); } + + /// Replays a complete play that happened offline / on a flaky + /// connection (#426 part B). One call: the server records start+end + /// from `atIso` (the original play-start time) + durationPlayedMs, + /// applies the canonical skip rule, and advances #415 rotation when + /// source is a system playlist. Driven by the offline mutation + /// queue, never the live path. + Future playOffline({ + required String trackId, + required String clientId, + required String atIso, + required int durationPlayedMs, + String? source, + }) async { + await _dio.post( + '/api/events', + data: { + 'type': 'play_offline', + 'track_id': trackId, + 'client_id': clientId, + 'at': atIso, + 'duration_played_ms': durationPlayedMs, + if (source != null && source.isNotEmpty) 'source': source, + }, + ); + } } diff --git a/flutter_client/lib/cache/mutation_queue.dart b/flutter_client/lib/cache/mutation_queue.dart index 7e7bb354..5077f892 100644 --- a/flutter_client/lib/cache/mutation_queue.dart +++ b/flutter_client/lib/cache/mutation_queue.dart @@ -30,6 +30,7 @@ import 'package:flutter/foundation.dart' show debugPrint; import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../api/endpoints/discover.dart'; +import '../api/endpoints/events.dart'; import '../api/endpoints/likes.dart'; import '../library/library_providers.dart' show dioProvider; import '../likes/likes_provider.dart' show likesApiProvider; @@ -52,6 +53,7 @@ class MutationKinds { static const playlistAppend = 'playlist.append'; static const requestCreate = 'request.create'; static const requestCancel = 'request.cancel'; + static const playOffline = 'play.offline'; } class MutationQueue { @@ -237,6 +239,8 @@ typedef _Handler = Future Function(Ref, Map); /// * playlist.append: {'playlistId': uuid, 'trackIds': [uuid, …]} /// * request.create: {full createRequest args; see DiscoverApi} /// * request.cancel: {'id': uuid} +/// * play.offline: {'trackId': uuid, 'clientId': str, 'at': iso8601, +/// 'durationPlayedMs': int, 'source'?: 'for_you'|'discover'} final Map _handlers = { MutationKinds.likeAdd: (ref, p) async { final api = await ref.read(likesApiProvider.future); @@ -281,6 +285,17 @@ final Map _handlers = { final api = await ref.read(requestsApiProvider.future); await api.cancel(p['id'] as String); }, + MutationKinds.playOffline: (ref, p) async { + final dio = await ref.read(dioProvider.future); + final api = EventsApi(dio); + await api.playOffline( + trackId: p['trackId'] as String, + clientId: p['clientId'] as String, + atIso: p['at'] as String, + durationPlayedMs: (p['durationPlayedMs'] as num).toInt(), + source: p['source'] as String?, + ); + }, }; LikeKind _likeKindFromString(String s) => switch (s) { diff --git a/flutter_client/lib/player/play_events_reporter.dart b/flutter_client/lib/player/play_events_reporter.dart index 3c3fb9dd..a06d2630 100644 --- a/flutter_client/lib/player/play_events_reporter.dart +++ b/flutter_client/lib/player/play_events_reporter.dart @@ -25,6 +25,8 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../api/endpoints/events.dart'; import '../auth/auth_provider.dart' show secureStorageProvider; +import '../cache/mutation_queue.dart' + show MutationKinds, mutationQueueProvider; import '../library/library_providers.dart' show dioProvider; import 'audio_handler.dart' show MinstrelAudioHandler; import 'player_provider.dart' show audioHandlerProvider; @@ -45,11 +47,21 @@ class PlayEventsReporter with WidgetsBindingObserver { String? _clientId; EventsApi? _api; + // Server play_event_id when the live play_started succeeded; null + // if start failed / offline — then the close is captured into the + // offline mutation queue instead of a live ended/skipped call. String? _openPlayEventId; - String? _openTrackId; + + // The play currently being tracked, captured independently of + // connectivity so an offline play is still a complete record. + String? _curTrackId; + DateTime? _curStartedAt; + String? _curSource; + int _curLastPositionMs = 0; + int _curDurationMs = 0; + bool _curReachedEnd = false; + String? _prevTrackId; - int _lastPositionMs = 0; - int _openDurationMs = 0; Future start() async { final MinstrelAudioHandler handler; @@ -69,13 +81,21 @@ class PlayEventsReporter with WidgetsBindingObserver { WidgetsBinding.instance.addObserver(this); _subs.add(handler.positionStream.listen((p) { - _lastPositionMs = p.inMilliseconds; - // Keep the open track's duration current while it's the one - // playing, so on a track change we still know how long the - // (now-prior) track was. + final ms = p.inMilliseconds; + // Advance the tracked play's progress ONLY while its track is + // the current one. A track change resets position to 0; gating + // on _curTrackId keeps the finishing track's last-known values + // intact for the close branch. final mi = handler.mediaItem.value; - if (mi != null && mi.id == _openTrackId && mi.duration != null) { - _openDurationMs = mi.duration!.inMilliseconds; + if (mi != null && mi.id == _curTrackId) { + _curLastPositionMs = ms; + final d = mi.duration; + if (d != null && d.inMilliseconds > 0) { + _curDurationMs = d.inMilliseconds; + if (ms >= _curDurationMs - _completionToleranceMs) { + _curReachedEnd = true; + } + } } })); _subs.add(handler.mediaItem.listen((_) => _evaluate(handler))); @@ -90,44 +110,42 @@ class PlayEventsReporter with WidgetsBindingObserver { final playing = st.playing; final completed = st.processingState == AudioProcessingState.completed; - // Track changed with an open row → close the prior row. Natural - // completion (reached ~duration) ends it; otherwise it's a skip. - if (tid != _prevTrackId && _openPlayEventId != null) { - final id = _openPlayEventId!; - final pos = _lastPositionMs; - _openPlayEventId = null; - _openTrackId = null; - final finished = _openDurationMs > 0 && - pos >= _openDurationMs - _completionToleranceMs; - if (finished) { - _api?.playEnded(playEventId: id, durationPlayedMs: pos) - .catchError((_) {}); - } else { - _api?.playSkipped(playEventId: id, positionMs: pos) - .catchError((_) {}); - } + // Track changed → close the prior tracked play. + if (tid != _prevTrackId && _curTrackId != null) { + _closeCurrent(viaOffline: false); } - // Entered playing for a new track → open a row. - if (tid != null && playing && _openTrackId != tid) { - _startNew(handler, tid); + // Entered playing for a new track → begin tracking it. + if (tid != null && playing && _curTrackId != tid) { + _beginTrack(handler, tid); } // Whole-queue natural end (just_audio only emits `completed` at - // the end of the sequence, not between items) → end the open row. - if (completed && _openPlayEventId != null) { - final id = _openPlayEventId!; - final dur = _lastPositionMs; - _openPlayEventId = null; - _openTrackId = null; - _api?.playEnded(playEventId: id, durationPlayedMs: dur) - .catchError((_) {}); + // the end of the sequence, not between items) → close it. + if (completed && _curTrackId != null) { + _curReachedEnd = true; + _closeCurrent(viaOffline: false); } _prevTrackId = tid; } - Future _startNew(MinstrelAudioHandler handler, String trackId) async { + void _beginTrack(MinstrelAudioHandler handler, String trackId) { + _curTrackId = trackId; + _curStartedAt = DateTime.now().toUtc(); + _curSource = handler.queueSource; + _curLastPositionMs = 0; + _curReachedEnd = false; + final d = handler.mediaItem.value?.duration; + _curDurationMs = d?.inMilliseconds ?? 0; + _openPlayEventId = null; + // Fire the live play_started; adopt the server id only if we're + // still on this track when the response lands. Failure is fine — + // the close path captures the whole play into the offline queue. + _startLive(handler, trackId); + } + + Future _startLive(MinstrelAudioHandler handler, String trackId) async { final api = _api; final cid = _clientId; if (api == null || cid == null) return; @@ -135,32 +153,93 @@ class PlayEventsReporter with WidgetsBindingObserver { final id = await api.playStarted( trackId: trackId, clientId: cid, - source: handler.queueSource, + source: _curSource, ); - // The user may have moved on by the time the response lands — - // only adopt the id if we're still on the same track. - if (id != null && handler.mediaItem.value?.id == trackId) { + if (id != null && _curTrackId == trackId) { _openPlayEventId = id; - _openTrackId = trackId; - final d = handler.mediaItem.value?.duration; - _openDurationMs = d?.inMilliseconds ?? 0; } } catch (_) { - // Best-effort; a missed event is acceptable per spec v1. + // Offline / flaky — _openPlayEventId stays null; the close path + // enqueues the completed play for replay. } } + /// Closes the currently-tracked play. `finished` is derived from + /// whether it reached ~its duration. If the live start registered a + /// server id we attempt the live ended/skipped close and fall back + /// to the offline queue on failure; with no server id (offline + /// start) — or viaOffline (app teardown, must be durable) — the + /// completed play is enqueued directly. The server's RecordOffline + /// Play applies the canonical skip rule, so the offline payload + /// only needs duration, not our finished/skipped guess. + void _closeCurrent({required bool viaOffline}) { + final trackId = _curTrackId; + final startedAt = _curStartedAt; + if (trackId == null || startedAt == null) { + _resetCurrent(); + return; + } + final reached = _curReachedEnd; + final lastPos = _curLastPositionMs; + final durationMs = (reached && _curDurationMs > 0) + ? _curDurationMs + : lastPos; + final source = _curSource; + final id = _openPlayEventId; + + if (!viaOffline && id != null) { + // Live close; on failure, fall back to the durable offline path + // so a transient blip at close time doesn't lose the play. + final fut = reached + ? _api?.playEnded(playEventId: id, durationPlayedMs: durationMs) + : _api?.playSkipped(playEventId: id, positionMs: lastPos); + fut?.catchError((_) { + _enqueueOffline(trackId, startedAt, source, durationMs); + }); + } else { + _enqueueOffline(trackId, startedAt, source, durationMs); + } + _resetCurrent(); + } + + void _resetCurrent() { + _curTrackId = null; + _curStartedAt = null; + _curSource = null; + _curLastPositionMs = 0; + _curDurationMs = 0; + _curReachedEnd = false; + _openPlayEventId = null; + } + + void _enqueueOffline( + String trackId, + DateTime startedAt, + String? source, + int durationPlayedMs, + ) { + final cid = _clientId; + if (cid == null) return; + // ignore: unawaited_futures + _ref.read(mutationQueueProvider).enqueue(MutationKinds.playOffline, { + 'trackId': trackId, + 'clientId': cid, + 'at': startedAt.toIso8601String(), + 'durationPlayedMs': durationPlayedMs, + if (source != null && source.isNotEmpty) 'source': source, + }); + } + @override void didChangeAppLifecycleState(AppLifecycleState state) { - // App backgrounded / killed with a live row: best-effort skip - // close so the row doesn't dangle. Mirrors web's pagehide beacon. + // App backgrounded / killed mid-play: close durably via the + // offline queue (a fire-and-forget POST during teardown is + // unreliable; the queue survives a process kill and drains on + // next launch). Mirrors the intent of web's pagehide beacon. if (state == AppLifecycleState.paused || state == AppLifecycleState.detached) { - final id = _openPlayEventId; - if (id != null) { - _api - ?.playSkipped(playEventId: id, positionMs: _lastPositionMs) - .catchError((_) {}); + if (_curTrackId != null) { + _closeCurrent(viaOffline: true); } } } From 1379595e82a271f559373bf921621f5ba012e4ab Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 12:25:54 -0400 Subject: [PATCH 13/27] =?UTF-8?q?refactor(playlists):=20#411=20R1=20?= =?UTF-8?q?=E2=80=94=20system-playlist=20kind=20registry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behavior-preserving prep for the new mix types. Extracts the three inline candidate computations in BuildSystemPlaylists into producers (produceForYou / produceSeedMixes / produceDiscover) and drives the build off a systemPlaylistRegistry. The shared machinery (run-claim guard, atomic delete+insert tx, post-commit collages) is now generic over a []builtPlaylist. Fatal-vs-skip error semantics unchanged: a base query failure (PickTopPlayedTracksForUser, PickSeedArtists) still aborts the whole build; candidate-load / per-seed-artist / Discover-bucket failures are still logged and just yield fewer playlists. Materialize order (for_you, songs_like_artist, discover) is unchanged and functionally irrelevant. No API/client/schema change — CI's system/foryou/service tests verify For You / Songs-like-X / Discover parity. Adding a new mix is now: a producer + one registry entry + its candidate query. Next (R2): generic /api/playlists/system/{kind}/{refresh,shuffle} off the registry; then the new kinds. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/playlists/system.go | 308 ++++++++++++++++++++--------------- 1 file changed, 178 insertions(+), 130 deletions(-) diff --git a/internal/playlists/system.go b/internal/playlists/system.go index 7bb03c20..faf8dc6d 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -225,6 +225,165 @@ func capCandidatesByAlbumAndArtist(cands []recommendation.Candidate) []recommend return out } +// builtPlaylist is what a producer hands back: one playlist to +// materialize. A producer may return zero (no eligible tracks), +// one (For-You / Discover / each new kind), or many (Songs-like-X, +// one per seed artist) of these. +type builtPlaylist struct { + Name string + Variant string + SeedArtistID pgtype.UUID // zero value for non-seeded kinds + Tracks []rankedCandidate +} + +// systemPlaylistProducer computes the playlists for one kind for a +// given user/day. A returned error is fatal to the whole build +// (e.g. a required base query failed); per-item issues should be +// logged and yield fewer playlists, not an error. +type systemPlaylistProducer func( + ctx context.Context, q *dbq.Queries, logger *slog.Logger, + userID pgtype.UUID, dateStr string, now time.Time, +) ([]builtPlaylist, error) + +type systemPlaylistKind struct { + Key string + Produce systemPlaylistProducer +} + +// systemPlaylistRegistry is the single source of truth for which +// system playlists exist. BuildSystemPlaylists iterates it; the +// generic /api/playlists/system/{kind}/{refresh,shuffle} endpoints +// validate against it. Adding a new mix = a producer + one entry +// here (plus its candidate query). Order is the materialize order; +// it has no functional effect (atomic replace + per-playlist +// collage are order-independent). +var systemPlaylistRegistry = []systemPlaylistKind{ + {Key: "for_you", Produce: produceForYou}, + {Key: "songs_like_artist", Produce: produceSeedMixes}, + {Key: "discover", Produce: produceDiscover}, +} + +// produceForYou: today's seed from the user's top-5 played tracks +// (rotates daily via userIDHash), similarity candidate pool, head+ +// tail composition. The base seed query failing is fatal; a +// candidate-load failure is logged and yields no For-You. +func produceForYou( + ctx context.Context, q *dbq.Queries, logger *slog.Logger, + userID pgtype.UUID, dateStr string, now time.Time, +) ([]builtPlaylist, error) { + forYouSeeds, err := q.PickTopPlayedTracksForUser(ctx, userID) + if err != nil { + return nil, fmt.Errorf("pick for-you seed candidates: %w", err) + } + forYouSeed := pickForYouSeedForDay(forYouSeeds, userID, dateStr) + if !forYouSeed.Valid { + return nil, nil + } + zeroVec := recommendation.SessionVector{Seed: true} + cands, cerr := recommendation.LoadCandidatesFromSimilarity( + ctx, q, userID, forYouSeed, + 1, // recentlyPlayedHours — small to avoid filtering the seed's recent neighbourhood + zeroVec, + []pgtype.UUID{forYouSeed}, + recommendation.DefaultCandidateSourceLimits(), + ) + if cerr != nil { + logger.Warn("system playlist: for-you candidates load failed; skipping", + "user_id", uuidStringPL(userID), "err", cerr) + return nil, nil + } + tracks := pickHeadAndTail(cands, userID, dateStr, now, forYouHeadN, forYouTailN) + if len(tracks) == 0 { + return nil, nil + } + return []builtPlaylist{{Name: "For You", Variant: "for_you", Tracks: tracks}}, nil +} + +// produceSeedMixes: up to 3 "Songs like {artist}" mixes. Seed +// artists rotate daily-deterministically. The base seed-artist +// query failing is fatal; per-artist failures are logged + skipped. +func produceSeedMixes( + ctx context.Context, q *dbq.Queries, logger *slog.Logger, + userID pgtype.UUID, dateStr string, now time.Time, +) ([]builtPlaylist, error) { + seedRows, err := q.PickSeedArtists(ctx, userID) + if err != nil { + return nil, fmt.Errorf("pick seed artists: %w", err) + } + seedRowsLocal := make([]seedArtistRow, 0, len(seedRows)) + for _, r := range seedRows { + seedRowsLocal = append(seedRowsLocal, seedArtistRow{ + ArtistID: r.ArtistID, Score: r.Score, + }) + } + seedPool := pickSeedArtistsFromRows(seedRowsLocal) + seeds := pickSeedArtistsForDay(seedPool, userID, dateStr) + + out := make([]builtPlaylist, 0, len(seeds)) + for _, artistID := range seeds { + artistRow, aerr := q.GetArtistByID(ctx, artistID) + if aerr != nil { + logger.Warn("system playlist: seed artist load failed; skipping", + "artist_id", uuidStringPL(artistID), "err", aerr) + continue + } + seedTrack, terr := q.PickTopPlayedTrackForArtistByUser(ctx, dbq.PickTopPlayedTrackForArtistByUserParams{ + UserID: userID, ArtistID: artistID, + }) + if terr != nil || !seedTrack.Valid { + continue + } + zeroVec := recommendation.SessionVector{Seed: true} + cands, cerr := recommendation.LoadCandidatesFromSimilarity( + ctx, q, userID, seedTrack, 1, zeroVec, []pgtype.UUID{seedTrack}, + recommendation.DefaultCandidateSourceLimits(), + ) + if cerr != nil { + logger.Warn("system playlist: seed candidates load failed; skipping", + "artist_id", uuidStringPL(artistID), "err", cerr) + continue + } + // "Songs like X" excludes X's own songs. + filtered := make([]recommendation.Candidate, 0, len(cands)) + for _, c := range cands { + if !pgtypeUUIDEqual(c.Track.ArtistID, artistID) { + filtered = append(filtered, c) + } + } + tracks := pickTopN(filtered, userID, dateStr, now, systemMixLength) + if len(tracks) == 0 { + continue + } + out = append(out, builtPlaylist{ + Name: fmt.Sprintf("Songs like %s", artistRow.Name), + Variant: "songs_like_artist", + SeedArtistID: artistID, + Tracks: tracks, + }) + } + return out, nil +} + +// produceDiscover: unheard tracks biased toward dormant artists. +// Bucket failures are handled inside buildDiscoverCandidates and +// redistribute as deficit; a returned error is non-fatal here +// (logged, yields no Discover) to match the prior behavior. +func produceDiscover( + ctx context.Context, q *dbq.Queries, logger *slog.Logger, + userID pgtype.UUID, dateStr string, now time.Time, +) ([]builtPlaylist, error) { + tracks, derr := buildDiscoverCandidates(ctx, q, logger, userID, dateStr) + if derr != nil { + logger.Warn("system playlist: discover candidates load failed; skipping", + "user_id", uuidStringPL(userID), "err", derr) + return nil, nil + } + if len(tracks) == 0 { + return nil, nil + } + return []builtPlaylist{{Name: "Discover", Variant: "discover", Tracks: tracks}}, nil +} + // BuildSystemPlaylists builds the user's daily system mixes (one For-You + // up to 3 Songs-like-{seed} mixes). Atomic-replace inside one tx; // concurrency-guarded via system_playlist_runs.in_flight; deterministic @@ -273,112 +432,22 @@ func BuildSystemPlaylists(ctx context.Context, pool *pgxpool.Pool, logger *slog. } }() - // 1. For-You: pick today's seed from the user's top-5 played tracks. - // Seed rotates daily via userIDHash so the candidate pool shifts - // across days while staying stable within a day. - forYouSeeds, err := q.PickTopPlayedTracksForUser(ctx, userID) - if err != nil { - buildErr = fmt.Errorf("pick for-you seed candidates: %w", err) - return buildErr - } - forYouSeed := pickForYouSeedForDay(forYouSeeds, userID, dateStr) - var forYouTracks []rankedCandidate - if forYouSeed.Valid { - zeroVec := recommendation.SessionVector{Seed: true} - cands, cerr := recommendation.LoadCandidatesFromSimilarity( - ctx, q, userID, forYouSeed, - 1, // recentlyPlayedHours — small to avoid filtering the seed's recent neighbourhood - zeroVec, - []pgtype.UUID{forYouSeed}, - recommendation.DefaultCandidateSourceLimits(), - ) - if cerr == nil { - // For-You uses head+tail composition: forYouHeadN top-similarity - // tracks + forYouTailN tail-sampled to inject daily-deterministic - // surprise. Songs-like-X keeps pickTopN (top-25 with caps) since - // the seed-artist context already provides the "you'll like this" - // framing. - forYouTracks = pickHeadAndTail(cands, userID, dateStr, now, forYouHeadN, forYouTailN) - } else { - logger.Warn("system playlist: for-you candidates load failed; skipping", - "user_id", uuidStringPL(userID), "err", cerr) + // Run every registered system-playlist producer. Each returns the + // playlists it wants materialized for this user/day (zero or more); + // a returned error is fatal to the whole build (matches the prior + // behavior where a seed-query failure aborted). Per-item issues are + // logged inside the producer and just yield fewer playlists. + var built []builtPlaylist + for _, kind := range systemPlaylistRegistry { + out, perr := kind.Produce(ctx, q, logger, userID, dateStr, now) + if perr != nil { + buildErr = fmt.Errorf("produce %s: %w", kind.Key, perr) + return buildErr } + built = append(built, out...) } - // 2. Seed artists for "Songs like {X}". SQL returns up to 5 candidates - // in score order; pickSeedArtistsForDay shuffles them daily- - // deterministically and takes 3 so the set of mixes rotates day-to-day. - seedRows, err := q.PickSeedArtists(ctx, userID) - if err != nil { - buildErr = fmt.Errorf("pick seed artists: %w", err) - return buildErr - } - seedRowsLocal := make([]seedArtistRow, 0, len(seedRows)) - for _, r := range seedRows { - seedRowsLocal = append(seedRowsLocal, seedArtistRow{ - ArtistID: r.ArtistID, Score: r.Score, - }) - } - seedPool := pickSeedArtistsFromRows(seedRowsLocal) - seeds := pickSeedArtistsForDay(seedPool, userID, dateStr) - - type seedMix struct { - ArtistID pgtype.UUID - ArtistName string - Tracks []rankedCandidate - } - seedMixes := make([]seedMix, 0, len(seeds)) - for _, artistID := range seeds { - artistRow, aerr := q.GetArtistByID(ctx, artistID) - if aerr != nil { - logger.Warn("system playlist: seed artist load failed; skipping", - "artist_id", uuidStringPL(artistID), "err", aerr) - continue - } - seedTrack, terr := q.PickTopPlayedTrackForArtistByUser(ctx, dbq.PickTopPlayedTrackForArtistByUserParams{ - UserID: userID, ArtistID: artistID, - }) - if terr != nil || !seedTrack.Valid { - continue - } - zeroVec := recommendation.SessionVector{Seed: true} - cands, cerr := recommendation.LoadCandidatesFromSimilarity( - ctx, q, userID, seedTrack, 1, zeroVec, []pgtype.UUID{seedTrack}, - recommendation.DefaultCandidateSourceLimits(), - ) - if cerr != nil { - logger.Warn("system playlist: seed candidates load failed; skipping", - "artist_id", uuidStringPL(artistID), "err", cerr) - continue - } - // Keep only tracks NOT by the seed artist (we want "songs like X", - // not X's own songs). - filtered := make([]recommendation.Candidate, 0, len(cands)) - for _, c := range cands { - if !pgtypeUUIDEqual(c.Track.ArtistID, artistID) { - filtered = append(filtered, c) - } - } - tracks := pickTopN(filtered, userID, dateStr, now, systemMixLength) - seedMixes = append(seedMixes, seedMix{ - ArtistID: artistID, - ArtistName: artistRow.Name, - Tracks: tracks, - }) - } - - // 4. Discover: surface unheard tracks, biased toward dormant artists. - // Individual bucket failures are logged inside buildDiscoverCandidates - // and redistribute as deficit; the function only returns an error for - // unrecoverable conditions, so a remaining derr here is genuine. - discoverTracks, derr := buildDiscoverCandidates(ctx, q, logger, userID, dateStr) - if derr != nil { - logger.Warn("system playlist: discover candidates load failed; skipping", - "user_id", uuidStringPL(userID), "err", derr) - discoverTracks = nil - } - - // 3. Atomic replace inside a transaction. + // Atomic replace inside a transaction. tx, err := pool.Begin(ctx) if err != nil { buildErr = fmt.Errorf("begin tx: %w", err) @@ -396,37 +465,16 @@ func BuildSystemPlaylists(ctx context.Context, pool *pgxpool.Pool, logger *slog. // for each one after the transaction commits. Collage generation // reads the playlist's tracks via a separate query that won't see // uncommitted rows, so the work happens post-commit. - createdIDs := make([]pgtype.UUID, 0, 1+len(seedMixes)) + createdIDs := make([]pgtype.UUID, 0, len(built)) - if len(forYouTracks) > 0 { - id, err := insertSystemPlaylist(ctx, qtx, userID, "For You", "for_you", - pgtype.UUID{}, forYouTracks) - if err != nil { - buildErr = fmt.Errorf("insert for-you: %w", err) - return buildErr - } - createdIDs = append(createdIDs, id) - } - - for _, m := range seedMixes { - if len(m.Tracks) == 0 { + for _, bp := range built { + if len(bp.Tracks) == 0 { continue } - name := fmt.Sprintf("Songs like %s", m.ArtistName) - id, err := insertSystemPlaylist(ctx, qtx, userID, name, "songs_like_artist", - m.ArtistID, m.Tracks) + id, err := insertSystemPlaylist(ctx, qtx, userID, bp.Name, bp.Variant, + bp.SeedArtistID, bp.Tracks) if err != nil { - buildErr = fmt.Errorf("insert songs-like %s: %w", uuidStringPL(m.ArtistID), err) - return buildErr - } - createdIDs = append(createdIDs, id) - } - - if len(discoverTracks) > 0 { - id, err := insertSystemPlaylist(ctx, qtx, userID, "Discover", "discover", - pgtype.UUID{}, discoverTracks) - if err != nil { - buildErr = fmt.Errorf("insert discover: %w", err) + buildErr = fmt.Errorf("insert %s (%s): %w", bp.Variant, bp.Name, err) return buildErr } createdIDs = append(createdIDs, id) From e3957b8eed7125ee1893a07c848bdfd263f5e33e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 12:33:05 -0400 Subject: [PATCH 14/27] fix(lint): rename unused now param in produceDiscover to _ revive unused-parameter: produceDiscover keys off dateStr, not now, but must keep the uniform systemPlaylistProducer signature. Blank the unused param (param names don't affect func-type identity). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/playlists/system.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/playlists/system.go b/internal/playlists/system.go index faf8dc6d..6e7cadc2 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -370,7 +370,7 @@ func produceSeedMixes( // (logged, yields no Discover) to match the prior behavior. func produceDiscover( ctx context.Context, q *dbq.Queries, logger *slog.Logger, - userID pgtype.UUID, dateStr string, now time.Time, + userID pgtype.UUID, dateStr string, _ time.Time, ) ([]builtPlaylist, error) { tracks, derr := buildDiscoverCandidates(ctx, q, logger, userID, dateStr) if derr != nil { From d67c0de5966d92c6706879f2f3bee266e9db4e3b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 13:21:09 -0400 Subject: [PATCH 15/27] =?UTF-8?q?refactor(playlists):=20#411=20R2=20?= =?UTF-8?q?=E2=80=94=20generic=20registry-driven=20system=20endpoints?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the per-kind refresh/shuffle handlers with one generic pair driven off the kind registry, in lockstep across both clients. Server: - systemPlaylistKind gains Singleton; RefreshableSystemKind(key) exported. for_you/discover singleton; songs_like_artist not. - New generic POST /api/playlists/system/{kind}/refresh and GET /api/playlists/system/{kind}/shuffle ({kind} = raw system_variant). Non-singleton/unknown kind → 404. Deleted playlists_{foryou,discover}_refresh.go and the per-kind shuffle wrappers; serveSystemPlaylistShuffle core kept. - playlistRowView.refreshable: server-derived flag so clients show the refresh affordance generically without hardcoding kinds. Web (not drift-cached → uses the server flag): - refreshSystem(variant) replaces refreshForYou/refreshDiscover; systemShuffle drops the for_you→for-you mapping (raw variant). - PlaylistCard + detail page gate the kebab/Refresh button on playlist.refreshable; label is "Refresh {name}". Tests reworked; obsolete refresh-foryou/discover api tests deleted. Flutter (list tiles are drift-cache-sourced → derive, no migration): - Playlist.refreshable getter = isSystem && variant != songs_like_artist (holds for all current + planned kinds). - refreshSystem/systemShuffle use the raw variant; PlaylistCard + detail screen gate kebab/Regenerate/source-tagging on refreshable so songs_like_artist plays via get() (no by-kind endpoint). Pure-plumbing refactor; CI verifies parity. Next (R3): the five discovery mixes — each a candidate query + one registry entry. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../lib/api/endpoints/playlists.dart | 25 ++--- flutter_client/lib/models/playlist.dart | 10 ++ .../lib/playlists/playlist_detail_screen.dart | 6 +- .../lib/playlists/widgets/playlist_card.dart | 28 ++--- internal/api/api.go | 6 +- internal/api/playlists.go | 8 +- internal/api/playlists_discover_refresh.go | 69 ------------ internal/api/playlists_foryou_refresh.go | 98 ---------------- internal/api/playlists_system_shuffle.go | 106 +++++++++++++++--- internal/playlists/system.go | 31 ++++- .../api/playlists.refresh-discover.test.ts | 28 ----- .../lib/api/playlists.refresh-foryou.test.ts | 28 ----- web/src/lib/api/playlists.ts | 54 +++------ web/src/lib/api/types.ts | 6 +- .../lib/components/AddToPlaylistMenu.test.ts | 3 + web/src/lib/components/PlaylistCard.svelte | 27 ++--- web/src/lib/components/PlaylistCard.test.ts | 100 +++++++---------- web/src/routes/page.test.ts | 1 + web/src/routes/playlists/[id]/+page.svelte | 26 ++--- .../routes/playlists/[id]/playlist.test.ts | 43 ++++--- web/src/routes/playlists/playlists.test.ts | 2 +- 21 files changed, 277 insertions(+), 428 deletions(-) delete mode 100644 internal/api/playlists_discover_refresh.go delete mode 100644 internal/api/playlists_foryou_refresh.go delete mode 100644 web/src/lib/api/playlists.refresh-discover.test.ts delete mode 100644 web/src/lib/api/playlists.refresh-foryou.test.ts diff --git a/flutter_client/lib/api/endpoints/playlists.dart b/flutter_client/lib/api/endpoints/playlists.dart index 92f6d49b..3455aaed 100644 --- a/flutter_client/lib/api/endpoints/playlists.dart +++ b/flutter_client/lib/api/endpoints/playlists.dart @@ -45,15 +45,13 @@ class PlaylistsApi { return PlaylistDetail.fromJson(r.data ?? const {}); } - /// GET /api/playlists/system/{variant}/shuffle (#415 stage 2/3). + /// GET /api/playlists/system/{kind}/shuffle (#415 / #411 R2). /// Same shape as get() but tracks are server-ordered rotation-aware - /// (unplayed-this-rotation first; resets when exhausted). Model - /// variant uses underscores; the route segment is hyphenated. - /// Intentionally uncached — varies per play. + /// (unplayed-this-rotation first; resets when exhausted). {kind} is + /// the raw system_variant. Intentionally uncached — varies per play. Future systemShuffle(String variant) async { - final seg = variant == 'for_you' ? 'for-you' : variant; final r = await _dio.get>( - '/api/playlists/system/$seg/shuffle', + '/api/playlists/system/$variant/shuffle', ); return PlaylistDetail.fromJson(r.data ?? const {}); } @@ -67,17 +65,14 @@ class PlaylistsApi { ); } - /// POST /api/playlists/system/{variant}/refresh. Synchronously - /// rebuilds the caller's system playlist for the given variant - /// ("for_you" | "discover") and returns the new playlist id, or - /// null when the library is empty so there's nothing to build. - /// - /// The variant uses underscores in the model (system_variant) but - /// the route segment is hyphenated ("for-you"), so map here. + /// POST /api/playlists/system/{kind}/refresh (#411 R2). Rebuilds + /// the caller's system playlists and returns the named kind's new + /// playlist id, or null when the library is empty. {kind} is the + /// raw system_variant — the server routes generically off the + /// kind registry, no hyphen mapping. Future refreshSystem(String variant) async { - final segment = variant == 'for_you' ? 'for-you' : variant; final r = await _dio.post>( - '/api/playlists/system/$segment/refresh', + '/api/playlists/system/$variant/refresh', data: const {}, ); return (r.data ?? const {})['playlist_id'] as String?; diff --git a/flutter_client/lib/models/playlist.dart b/flutter_client/lib/models/playlist.dart index 7916fde7..1ae01860 100644 --- a/flutter_client/lib/models/playlist.dart +++ b/flutter_client/lib/models/playlist.dart @@ -38,6 +38,16 @@ class Playlist { bool get isSystem => systemVariant != null; + /// Whether this playlist supports the generic by-kind refresh/ + /// shuffle endpoints (#411 R2) — i.e. a singleton system kind. + /// The server exposes a `refreshable` flag for JSON-sourced + /// playlists, but the list tiles are drift-cache-sourced (no + /// migration just for this), so derive it: every system kind is a + /// singleton except songs_like_artist (multi-per-user). This rule + /// holds for For-You/Discover and all planned discovery mixes; if + /// a future non-singleton kind appears, extend the exclusion. + bool get refreshable => isSystem && systemVariant != 'songs_like_artist'; + factory Playlist.fromJson(Map j) => Playlist( id: j['id'] as String, userId: j['user_id'] as String? ?? '', diff --git a/flutter_client/lib/playlists/playlist_detail_screen.dart b/flutter_client/lib/playlists/playlist_detail_screen.dart index 7f0f59d8..b53cc7d2 100644 --- a/flutter_client/lib/playlists/playlist_detail_screen.dart +++ b/flutter_client/lib/playlists/playlist_detail_screen.dart @@ -212,7 +212,7 @@ class _Body extends ConsumerWidget { ref.read(playerActionsProvider).playTracks( playableRefs, initialIndex: startIdx >= 0 ? startIdx : 0, - source: detail.playlist.isSystem + source: detail.playlist.refreshable ? detail.playlist.systemVariant : null, ); @@ -266,7 +266,7 @@ class _Header extends ConsumerWidget { ), const Spacer(), if (playable.isNotEmpty) ...[ - if (p.isSystem) + if (p.refreshable) OutlinedButton.icon( key: const Key('regenerate_playlist_button'), onPressed: () => _regenerate(context, ref), @@ -298,7 +298,7 @@ class _Header extends ConsumerWidget { final refs = playable.map(_toTrackRef).toList(growable: false); ref.read(playerActionsProvider).playTracks( refs, - source: p.isSystem ? p.systemVariant : null, + source: p.refreshable ? p.systemVariant : null, ); }, icon: const Icon(Icons.play_arrow), diff --git a/flutter_client/lib/playlists/widgets/playlist_card.dart b/flutter_client/lib/playlists/widgets/playlist_card.dart index 0026834b..16cfaef0 100644 --- a/flutter_client/lib/playlists/widgets/playlist_card.dart +++ b/flutter_client/lib/playlists/widgets/playlist_card.dart @@ -69,7 +69,7 @@ class PlaylistCard extends ConsumerWidget { // System playlists get a refresh affordance so the // user can force a fresh mix instead of waiting for // the daily 03:00 rebuild. Mirrors the web kebab. - if (playlist.isSystem) + if (playlist.refreshable) Positioned( top: 2, right: 2, @@ -123,11 +123,7 @@ class PlaylistCard extends ConsumerWidget { ); } - String get _refreshLabel => switch (playlist.systemVariant) { - 'for_you' => 'Refresh For You', - 'discover' => 'Refresh Discover', - _ => 'Refresh', - }; + String get _refreshLabel => 'Refresh ${playlist.name}'; /// Forces a server-side rebuild of this system playlist, then /// invalidates the aggregate list so the rotated UUID + new tracks @@ -155,11 +151,12 @@ class PlaylistCard extends ConsumerWidget { /// index 0. Mirrors the web PlaylistCard's onPlayClick. Future _playPlaylist(WidgetRef ref) async { final api = await ref.read(playlistsApiProvider.future); - // System playlists: fetch the server's rotation-aware order - // (#415) and play it as-is, tagged with the source so the - // play-events reporter advances that playlist's rotation. User - // playlists keep stored order, untagged. - final detail = playlist.isSystem + // Refreshable (singleton) system playlists: fetch the server's + // rotation-aware order (#415) and play as-is, tagged with the + // source so the reporter advances rotation. User playlists AND + // non-singleton system kinds (songs_like_artist — no by-kind + // endpoint) play the stored order via get(), untagged. + final detail = playlist.refreshable ? await api.systemShuffle(playlist.systemVariant!) : await api.get(playlist.id); final refs = []; @@ -177,14 +174,13 @@ class PlaylistCard extends ConsumerWidget { )); } if (refs.isEmpty) return; - // System playlists already arrive in rotation-aware order from - // the server (#415) — play as-is, tagged with the variant so the - // reporter advances rotation. No client shuffle. User playlists - // play in stored order, untagged. + // Rotation-aware kinds arrive pre-ordered from the server — play + // as-is, tagged so the reporter advances rotation. No client + // shuffle. Everything else plays stored order, untagged. await ref.read(playerActionsProvider).playTracks( refs, initialIndex: 0, - source: playlist.isSystem ? playlist.systemVariant : null, + source: playlist.refreshable ? playlist.systemVariant : null, ); } } diff --git a/internal/api/api.go b/internal/api/api.go index 3cbf2dbf..52878972 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -172,10 +172,8 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Delete("/playlists/{id}/tracks/{position}", h.handleRemovePlaylistTrack) authed.Put("/playlists/{id}/tracks", h.handleReorderPlaylist) authed.Get("/playlists/{id}/cover", h.handleGetPlaylistCover) - authed.Post("/playlists/system/discover/refresh", h.handleDiscoverRefresh) - authed.Post("/playlists/system/for-you/refresh", h.handleForYouRefresh) - authed.Get("/playlists/system/discover/shuffle", h.handleDiscoverShuffle) - authed.Get("/playlists/system/for-you/shuffle", h.handleForYouShuffle) + authed.Post("/playlists/system/{kind}/refresh", h.handleSystemPlaylistRefresh) + authed.Get("/playlists/system/{kind}/shuffle", h.handleSystemPlaylistShuffle) }) }) } diff --git a/internal/api/playlists.go b/internal/api/playlists.go index 1a94f5a9..bd2779a0 100644 --- a/internal/api/playlists.go +++ b/internal/api/playlists.go @@ -32,7 +32,12 @@ type playlistRowView struct { IsPublic bool `json:"is_public"` Kind string `json:"kind"` SystemVariant *string `json:"system_variant,omitempty"` - SeedArtistID *string `json:"seed_artist_id,omitempty"` + // Refreshable: a singleton system kind addressable by the generic + // /system/{kind}/{refresh,shuffle} endpoints. Lets clients show + // the per-tile refresh affordance generically without hardcoding + // which kinds support it (false for user + songs_like_artist). + Refreshable bool `json:"refreshable"` + SeedArtistID *string `json:"seed_artist_id,omitempty"` CoverURL string `json:"cover_url,omitempty"` TrackCount int32 `json:"track_count"` DurationSec int32 `json:"duration_sec"` @@ -435,6 +440,7 @@ func playlistRowToView(r *playlists.PlaylistRow) playlistRowView { IsPublic: r.IsPublic, Kind: r.Kind, SystemVariant: r.SystemVariant, + Refreshable: r.SystemVariant != nil && playlists.RefreshableSystemKind(*r.SystemVariant), TrackCount: r.TrackCount, DurationSec: r.DurationSec, CreatedAt: formatTimestamp(r.CreatedAt), diff --git a/internal/api/playlists_discover_refresh.go b/internal/api/playlists_discover_refresh.go deleted file mode 100644 index 7ec2cf9f..00000000 --- a/internal/api/playlists_discover_refresh.go +++ /dev/null @@ -1,69 +0,0 @@ -package api - -import ( - "errors" - "net/http" - "time" - - "github.com/jackc/pgx/v5" - - "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" - "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/playlists" -) - -// discoverRefreshResp is the wire shape for POST -// /api/playlists/system/discover/refresh. playlist_id is null when -// the build succeeded but the user's library yielded no eligible -// tracks (degenerate empty-library); track_count is 0 in that case. -type discoverRefreshResp struct { - PlaylistID *string `json:"playlist_id"` - TrackCount int32 `json:"track_count"` -} - -// handleDiscoverRefresh re-runs BuildSystemPlaylists synchronously -// for the calling user, then returns the resulting Discover row's -// id + track_count. Used by the frontend's refresh affordances on -// the detail page header and the home tile kebab. -// -// Authenticated user only (gated by the authed sub-router's -// RequireUser middleware). Each user only ever refreshes their own -// Discover — no admin route, no cross-user impersonation. -func (h *handlers) handleDiscoverRefresh(w http.ResponseWriter, r *http.Request) { - user, ok := requireUser(w, r) - if !ok { - return - } - if err := playlists.BuildSystemPlaylists(r.Context(), h.pool, h.logger, user.ID, time.Now(), h.dataDir); err != nil { - h.logger.Error("discover refresh: build failed", - "user_id", uuidToString(user.ID), "err", err) - writeErr(w, apierror.InternalMsg("build failed", err)) - return - } - - sysVariant := "discover" - pl, err := dbq.New(h.pool).GetSystemPlaylistByVariantForUser(r.Context(), - dbq.GetSystemPlaylistByVariantForUserParams{ - UserID: user.ID, - SystemVariant: &sysVariant, - }) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - // Build succeeded but no Discover row produced — empty library. - writeJSON(w, http.StatusOK, discoverRefreshResp{ - PlaylistID: nil, TrackCount: 0, - }) - return - } - h.logger.Error("discover refresh: lookup failed", - "user_id", uuidToString(user.ID), "err", err) - writeErr(w, apierror.InternalMsg("lookup failed", err)) - return - } - - idStr := uuidToString(pl.ID) - writeJSON(w, http.StatusOK, discoverRefreshResp{ - PlaylistID: &idStr, - TrackCount: pl.TrackCount, - }) -} diff --git a/internal/api/playlists_foryou_refresh.go b/internal/api/playlists_foryou_refresh.go deleted file mode 100644 index eee83f8f..00000000 --- a/internal/api/playlists_foryou_refresh.go +++ /dev/null @@ -1,98 +0,0 @@ -package api - -import ( - "errors" - "net/http" - "time" - - "github.com/jackc/pgx/v5" - - "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" - "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/playlists" -) - -// foryouRefreshResp is the wire shape for POST -// /api/playlists/system/for-you/refresh. track_ids is the playlist's -// tracks in playlist position order — the frontend enqueues this -// list directly to start playback without a second roundtrip. -// -// playlist_id is null + track_ids empty when the build succeeded but -// the user's library yielded no eligible candidates (degenerate -// empty-library case). -type foryouRefreshResp struct { - PlaylistID *string `json:"playlist_id"` - TrackCount int32 `json:"track_count"` - TrackIDs []string `json:"track_ids"` -} - -// handleForYouRefresh re-runs BuildSystemPlaylists synchronously for -// the calling user and returns their freshly-built For-You playlist. -// Used by the home Playlists row's For-You tile play button: click -// triggers refresh, the response carries the track IDs, the -// frontend enqueues + auto-plays. -// -// Authenticated user only — each user refreshes only their own -// For-You. -func (h *handlers) handleForYouRefresh(w http.ResponseWriter, r *http.Request) { - user, ok := requireUser(w, r) - if !ok { - return - } - if err := playlists.BuildSystemPlaylists(r.Context(), h.pool, h.logger, user.ID, time.Now(), h.dataDir); err != nil { - h.logger.Error("for-you refresh: build failed", - "user_id", uuidToString(user.ID), "err", err) - writeErr(w, apierror.InternalMsg("build failed", err)) - return - } - - q := dbq.New(h.pool) - sysVariant := "for_you" - pl, err := q.GetSystemPlaylistByVariantForUser(r.Context(), - dbq.GetSystemPlaylistByVariantForUserParams{ - UserID: user.ID, - SystemVariant: &sysVariant, - }) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - // Build succeeded but no For-You row — empty library. - writeJSON(w, http.StatusOK, foryouRefreshResp{ - PlaylistID: nil, - TrackCount: 0, - TrackIDs: []string{}, - }) - return - } - h.logger.Error("for-you refresh: lookup failed", - "user_id", uuidToString(user.ID), "err", err) - writeErr(w, apierror.InternalMsg("lookup failed", err)) - return - } - - // Fetch tracks in playlist position order so the frontend can - // enqueue them directly. - rows, err := q.ListPlaylistTracks(r.Context(), pl.ID) - if err != nil { - h.logger.Error("for-you refresh: list tracks failed", - "playlist_id", uuidToString(pl.ID), "err", err) - writeErr(w, apierror.InternalMsg("list tracks failed", err)) - return - } - trackIDs := make([]string, 0, len(rows)) - for _, row := range rows { - // Skip rows where the track was removed from the library - // (LiveTrackID won't be Valid). The snapshot row still - // exists in playlist_tracks, but there's nothing to enqueue. - if !row.LiveTrackID.Valid { - continue - } - trackIDs = append(trackIDs, uuidToString(row.LiveTrackID)) - } - - idStr := uuidToString(pl.ID) - writeJSON(w, http.StatusOK, foryouRefreshResp{ - PlaylistID: &idStr, - TrackCount: pl.TrackCount, - TrackIDs: trackIDs, - }) -} diff --git a/internal/api/playlists_system_shuffle.go b/internal/api/playlists_system_shuffle.go index e531f3f0..bca19717 100644 --- a/internal/api/playlists_system_shuffle.go +++ b/internal/api/playlists_system_shuffle.go @@ -4,7 +4,9 @@ import ( "errors" "math/rand" "net/http" + "time" + "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" @@ -12,27 +14,99 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/playlists" ) -// Stage 2 of #415. GET /api/playlists/system/{variant}/shuffle returns -// the caller's system playlist with tracks in rotation-aware play -// order: tracks not yet heard this rotation first (shuffled), then -// already-heard tracks (shuffled). When the whole snapshot has been -// heard the rotation resets and the full list reshuffles. +// Generic registry-driven system-playlist endpoints (#411 R2), +// replacing the former per-kind handlers. {kind} is the raw +// system_variant ('for_you' | 'discover' | future singleton kinds); +// only singleton kinds (playlists.RefreshableSystemKind) are +// addressable here — songs_like_artist is multi-per-user and played +// by playlist id, so it 404s. // -// Same JSON shape as GET /api/playlists/{id} so clients reuse their -// existing track parsing. Intentionally a separate, uncached endpoint -// (Option A): the cached playlist-detail GET stays pure for the -// "open the playlist to look at it" path; this one varies per play. +// GET /api/playlists/system/{kind}/shuffle — the caller's playlist +// for that kind with tracks in rotation-aware order (#415): unheard +// this rotation first (shuffled), then heard; resets when exhausted. +// Same JSON shape as GET /api/playlists/{id} so clients reuse track +// parsing. Intentionally uncached — it varies per play, unlike the +// cached detail GET. // -// Two explicit static routes (one per variant) mirror the refresh -// handlers and sidestep chi static-vs-param ambiguity under -// /playlists/system/. Future kinds add their own thin route. +// POST /api/playlists/system/{kind}/refresh — rebuilds the caller's +// system playlists and returns that kind's fresh row. -func (h *handlers) handleForYouShuffle(w http.ResponseWriter, r *http.Request) { - h.serveSystemPlaylistShuffle(w, r, "for_you") +func systemKindFromURL(w http.ResponseWriter, r *http.Request) (string, bool) { + kind := chi.URLParam(r, "kind") + if !playlists.RefreshableSystemKind(kind) { + writeErr(w, &apierror.Error{Status: 404, Code: "not_found", Message: "unknown system playlist"}) + return "", false + } + return kind, true } -func (h *handlers) handleDiscoverShuffle(w http.ResponseWriter, r *http.Request) { - h.serveSystemPlaylistShuffle(w, r, "discover") +func (h *handlers) handleSystemPlaylistShuffle(w http.ResponseWriter, r *http.Request) { + kind, ok := systemKindFromURL(w, r) + if !ok { + return + } + h.serveSystemPlaylistShuffle(w, r, kind) +} + +// systemRefreshResp unifies the prior for-you/discover shapes +// (for-you carried track_ids, discover didn't); clients ignore +// extra fields. playlist_id null + empty when the build produced no +// row for that kind (empty library). +type systemRefreshResp struct { + PlaylistID *string `json:"playlist_id"` + TrackCount int32 `json:"track_count"` + TrackIDs []string `json:"track_ids"` +} + +func (h *handlers) handleSystemPlaylistRefresh(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + kind, ok := systemKindFromURL(w, r) + if !ok { + return + } + if err := playlists.BuildSystemPlaylists(r.Context(), h.pool, h.logger, user.ID, time.Now(), h.dataDir); err != nil { + h.logger.Error("system refresh: build failed", + "kind", kind, "user_id", uuidToString(user.ID), "err", err) + writeErr(w, apierror.InternalMsg("build failed", err)) + return + } + q := dbq.New(h.pool) + v := kind + pl, err := q.GetSystemPlaylistByVariantForUser(r.Context(), + dbq.GetSystemPlaylistByVariantForUserParams{UserID: user.ID, SystemVariant: &v}) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + writeJSON(w, http.StatusOK, systemRefreshResp{TrackIDs: []string{}}) + return + } + h.logger.Error("system refresh: lookup failed", + "kind", kind, "user_id", uuidToString(user.ID), "err", err) + writeErr(w, apierror.InternalMsg("lookup failed", err)) + return + } + rows, err := q.ListPlaylistTracks(r.Context(), pl.ID) + if err != nil { + h.logger.Error("system refresh: list tracks failed", + "kind", kind, "playlist_id", uuidToString(pl.ID), "err", err) + writeErr(w, apierror.InternalMsg("list tracks failed", err)) + return + } + trackIDs := make([]string, 0, len(rows)) + for _, row := range rows { + if !row.LiveTrackID.Valid { + continue + } + trackIDs = append(trackIDs, uuidToString(row.LiveTrackID)) + } + idStr := uuidToString(pl.ID) + writeJSON(w, http.StatusOK, systemRefreshResp{ + PlaylistID: &idStr, + TrackCount: pl.TrackCount, + TrackIDs: trackIDs, + }) } func (h *handlers) serveSystemPlaylistShuffle(w http.ResponseWriter, r *http.Request, variant string) { diff --git a/internal/playlists/system.go b/internal/playlists/system.go index 6e7cadc2..97b97e8f 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -246,8 +246,29 @@ type systemPlaylistProducer func( ) ([]builtPlaylist, error) type systemPlaylistKind struct { - Key string - Produce systemPlaylistProducer + Key string + // Singleton kinds produce exactly one playlist per user (For-You, + // Discover, the discovery mixes) and so can be addressed by kind: + // the generic /system/{kind}/{refresh,shuffle} endpoints + the + // per-tile refresh affordance only apply to these. Non-singleton + // kinds (songs_like_artist → up to 3 per user) are played by + // playlist id and have no by-kind endpoint. + Singleton bool + Produce systemPlaylistProducer +} + +// RefreshableSystemKind reports whether `key` is a known singleton +// system-playlist kind — i.e. addressable by the generic by-kind +// refresh/shuffle endpoints and eligible for the per-tile refresh +// affordance. The API layer + clients use this so neither hardcodes +// the kind list. +func RefreshableSystemKind(key string) bool { + for _, k := range systemPlaylistRegistry { + if k.Key == key { + return k.Singleton + } + } + return false } // systemPlaylistRegistry is the single source of truth for which @@ -258,9 +279,9 @@ type systemPlaylistKind struct { // it has no functional effect (atomic replace + per-playlist // collage are order-independent). var systemPlaylistRegistry = []systemPlaylistKind{ - {Key: "for_you", Produce: produceForYou}, - {Key: "songs_like_artist", Produce: produceSeedMixes}, - {Key: "discover", Produce: produceDiscover}, + {Key: "for_you", Singleton: true, Produce: produceForYou}, + {Key: "songs_like_artist", Singleton: false, Produce: produceSeedMixes}, + {Key: "discover", Singleton: true, Produce: produceDiscover}, } // produceForYou: today's seed from the user's top-5 played tracks diff --git a/web/src/lib/api/playlists.refresh-discover.test.ts b/web/src/lib/api/playlists.refresh-discover.test.ts deleted file mode 100644 index 98b72e43..00000000 --- a/web/src/lib/api/playlists.refresh-discover.test.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { refreshDiscover } from './playlists'; - -vi.mock('./client', () => ({ - api: { post: vi.fn() } -})); - -import { api } from './client'; - -describe('refreshDiscover', () => { - beforeEach(() => vi.clearAllMocks()); - - it('POSTs the correct path and returns the response', async () => { - const sample = { playlist_id: 'abc-123', track_count: 100 }; - (api.post as unknown as ReturnType).mockResolvedValueOnce(sample); - const got = await refreshDiscover(); - expect(api.post).toHaveBeenCalledWith('/api/playlists/system/discover/refresh', {}); - expect(got).toEqual(sample); - }); - - it('handles empty-library response', async () => { - const sample = { playlist_id: null, track_count: 0 }; - (api.post as unknown as ReturnType).mockResolvedValueOnce(sample); - const got = await refreshDiscover(); - expect(got.playlist_id).toBe(null); - expect(got.track_count).toBe(0); - }); -}); diff --git a/web/src/lib/api/playlists.refresh-foryou.test.ts b/web/src/lib/api/playlists.refresh-foryou.test.ts deleted file mode 100644 index de715232..00000000 --- a/web/src/lib/api/playlists.refresh-foryou.test.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { refreshForYou } from './playlists'; - -vi.mock('./client', () => ({ - api: { post: vi.fn() } -})); - -import { api } from './client'; - -describe('refreshForYou', () => { - beforeEach(() => vi.clearAllMocks()); - - it('POSTs the correct path and returns the response', async () => { - const sample = { playlist_id: 'pl-abc', track_count: 25, track_ids: ['t1', 't2'] }; - (api.post as unknown as ReturnType).mockResolvedValueOnce(sample); - const got = await refreshForYou(); - expect(api.post).toHaveBeenCalledWith('/api/playlists/system/for-you/refresh', {}); - expect(got).toEqual(sample); - }); - - it('handles empty-library response', async () => { - const sample = { playlist_id: null, track_count: 0, track_ids: [] }; - (api.post as unknown as ReturnType).mockResolvedValueOnce(sample); - const got = await refreshForYou(); - expect(got.playlist_id).toBe(null); - expect(got.track_ids).toEqual([]); - }); -}); diff --git a/web/src/lib/api/playlists.ts b/web/src/lib/api/playlists.ts index e1dfa192..bd340d74 100644 --- a/web/src/lib/api/playlists.ts +++ b/web/src/lib/api/playlists.ts @@ -21,13 +21,27 @@ export async function getPlaylist(id: string): Promise { // #415 stage 2/3: rotation-aware play order for a system playlist. // Same shape as getPlaylist but tracks are server-ordered (unplayed -// this rotation first, then heard; resets when exhausted). The model -// variant uses underscores; the route segment is hyphenated. +// this rotation first, then heard; resets when exhausted). // Intentionally uncached — it varies per play, unlike getPlaylist. +// {variant} is the raw system_variant (#411 R2: generic by-kind). export async function systemShuffle(variant: string): Promise { - const seg = variant === 'for_you' ? 'for-you' : variant; return api.get( - `/api/playlists/system/${encodeURIComponent(seg)}/shuffle`, + `/api/playlists/system/${encodeURIComponent(variant)}/shuffle`, + ); +} + +// #411 R2: generic by-kind refresh. Rebuilds the caller's system +// playlists and returns the named kind's fresh row. Replaces the +// former per-kind refreshForYou/refreshDiscover. +export type RefreshSystemResponse = { + playlist_id: string | null; + track_count: number; + track_ids: string[]; +}; +export async function refreshSystem(variant: string): Promise { + return api.post( + `/api/playlists/system/${encodeURIComponent(variant)}/refresh`, + {}, ); } @@ -91,35 +105,3 @@ export function createPlaylistQuery(id: string) { }); } -// System playlist refresh ---------------------------------------------------- - -export type RefreshDiscoverResponse = { - playlist_id: string | null; - track_count: number; -}; - -// Re-runs the daily system playlist build for the calling user and -// returns the resulting Discover playlist's id + track count. -// Used by the detail-page Refresh button and the home tile kebab. -export async function refreshDiscover(): Promise { - return api.post('/api/playlists/system/discover/refresh', {}); -} - -// For-You refresh ------------------------------------------------------------ - -export type RefreshForYouResponse = { - playlist_id: string | null; - track_count: number; - track_ids: string[]; -}; - -// POST /api/playlists/system/for-you/refresh — synchronously -// rebuilds the calling user's For-You playlist and returns the new -// id, track count, and track id list. -// -// Used by the home Playlists row's For-You tile play button: click -// triggers refresh, the frontend then calls getPlaylist(new_id) to -// get full TrackRef snapshots and enqueues for playback. -export async function refreshForYou(): Promise { - return api.post('/api/playlists/system/for-you/refresh', {}); -} diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index c58ded2f..d4235b42 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -59,7 +59,11 @@ export type Playlist = { description: string; is_public: boolean; kind: 'user' | 'system'; - system_variant: 'for_you' | 'songs_like_artist' | 'discover' | null; + system_variant: string | null; + // Server-derived: a singleton system kind addressable by the + // generic /system/{kind}/{refresh,shuffle} endpoints. Drives the + // per-tile refresh affordance without the client hardcoding kinds. + refreshable: boolean; seed_artist_id: string | null; cover_url: string; // empty string when no cover; UI renders glyph track_count: number; diff --git a/web/src/lib/components/AddToPlaylistMenu.test.ts b/web/src/lib/components/AddToPlaylistMenu.test.ts index 89ae38e5..8f0a123b 100644 --- a/web/src/lib/components/AddToPlaylistMenu.test.ts +++ b/web/src/lib/components/AddToPlaylistMenu.test.ts @@ -14,6 +14,7 @@ const playlistsData = vi.hoisted(() => ({ is_public: false, kind: 'user', system_variant: null, + refreshable: false, seed_artist_id: null, cover_url: '', track_count: 3, @@ -30,6 +31,7 @@ const playlistsData = vi.hoisted(() => ({ is_public: false, kind: 'user', system_variant: null, + refreshable: false, seed_artist_id: null, cover_url: '', track_count: 5, @@ -58,6 +60,7 @@ vi.mock('$lib/api/playlists', () => ({ is_public: false, kind: 'user', system_variant: null, + refreshable: false, seed_artist_id: null, cover_url: '', track_count: 0, diff --git a/web/src/lib/components/PlaylistCard.svelte b/web/src/lib/components/PlaylistCard.svelte index 92f41875..1436afca 100644 --- a/web/src/lib/components/PlaylistCard.svelte +++ b/web/src/lib/components/PlaylistCard.svelte @@ -3,7 +3,7 @@ import { useQueryClient } from '@tanstack/svelte-query'; import type { Playlist, PlaylistTrack, TrackRef } from '$lib/api/types'; import { user } from '$lib/auth/store.svelte'; - import { getPlaylist, systemShuffle, refreshDiscover, refreshForYou } from '$lib/api/playlists'; + import { getPlaylist, systemShuffle, refreshSystem } from '$lib/api/playlists'; import { playlistTrackToRef } from '$lib/playlists/playlistTrackToRef'; import { errCode } from '$lib/api/errors'; import { qk } from '$lib/api/queries'; @@ -13,12 +13,9 @@ let { playlist }: { playlist: Playlist } = $props(); const isOwn = $derived(user.value?.id === playlist.user_id); - const isDiscover = $derived(playlist.system_variant === 'discover'); - const isForYou = $derived(playlist.system_variant === 'for_you'); - const isSystem = $derived(playlist.system_variant != null); - const refreshLabel = $derived( - isDiscover ? 'Refresh Discover' : isForYou ? 'Refresh For You' : 'Refresh', - ); + // Server tells us which system playlists support by-kind refresh + // (#411 R2) — no hardcoded kind list here. + const refreshable = $derived(playlist.refreshable === true); const queryClient = useQueryClient(); @@ -72,16 +69,10 @@ e.preventDefault(); e.stopPropagation(); menuOpen = false; + if (!refreshable || playlist.system_variant == null) return; try { - if (isDiscover) { - await refreshDiscover(); - pushToast('Discover refreshed.'); - } else if (isForYou) { - await refreshForYou(); - pushToast('For You refreshed.'); - } else { - return; - } + await refreshSystem(playlist.system_variant); + pushToast(`${playlist.name} refreshed.`); await queryClient.invalidateQueries({ queryKey: qk.playlist(playlist.id) }); await queryClient.invalidateQueries({ queryKey: qk.playlists() }); } catch (err: unknown) { @@ -93,7 +84,7 @@ { menuOpen = false; }} />
- {#if isSystem} + {#if refreshable}
{/if} diff --git a/web/src/lib/components/PlaylistCard.test.ts b/web/src/lib/components/PlaylistCard.test.ts index 5eac8386..c5d3bf3c 100644 --- a/web/src/lib/components/PlaylistCard.test.ts +++ b/web/src/lib/components/PlaylistCard.test.ts @@ -17,6 +17,7 @@ vi.mock('$lib/api/playlists', () => ({ is_public: false, kind: 'user', system_variant: null, + refreshable: false, seed_artist_id: null, cover_url: '', track_count: 1, @@ -47,6 +48,7 @@ vi.mock('$lib/api/playlists', () => ({ is_public: false, kind: 'system', system_variant: 'for_you', + refreshable: true, seed_artist_id: null, cover_url: '', track_count: 1, @@ -68,8 +70,7 @@ vi.mock('$lib/api/playlists', () => ({ } ] } as PlaylistDetail), - refreshDiscover: vi.fn().mockResolvedValue({ playlist_id: 'p-1', track_count: 5 }), - refreshForYou: vi.fn().mockResolvedValue({ playlist_id: 'p-new', track_count: 25, track_ids: ['t-1'] }) + refreshSystem: vi.fn().mockResolvedValue({ playlist_id: 'p-1', track_count: 5, track_ids: ['t-1'] }) })); vi.mock('$lib/player/store.svelte', () => ({ @@ -92,6 +93,7 @@ const base: Playlist = { is_public: false, kind: 'user', system_variant: null, + refreshable: false, seed_artist_id: null, cover_url: '', track_count: 12, @@ -173,98 +175,73 @@ describe('PlaylistCard', () => { expect(playQueue).toHaveBeenCalled(); }); - test('shows kebab button on Discover playlists', () => { + test('shows kebab on refreshable (singleton system) playlists', () => { const discoverPlaylist: Playlist = { ...base, kind: 'system', system_variant: 'discover', + refreshable: true, name: 'Discover' }; render(PlaylistCard, { props: { playlist: discoverPlaylist } }); expect(screen.getByLabelText(/playlist actions/i)).toBeInTheDocument(); }); - test('shows kebab button on For You playlists', () => { - const forYouPlaylist: Playlist = { - ...base, - kind: 'system', - system_variant: 'for_you', - name: 'For You' - }; - render(PlaylistCard, { props: { playlist: forYouPlaylist } }); - expect(screen.getByLabelText(/playlist actions/i)).toBeInTheDocument(); - }); - - test('does not show kebab button on user playlists', () => { + test('does not show kebab on user playlists', () => { render(PlaylistCard, { props: { playlist: base } }); expect(screen.queryByLabelText(/playlist actions/i)).not.toBeInTheDocument(); }); - test('Refresh Discover item appears in Discover kebab', async () => { + test('does not show kebab on a non-refreshable system playlist', () => { + // e.g. songs_like_artist — system but multi-per-user, server + // reports refreshable:false. + const songsLike: Playlist = { + ...base, + kind: 'system', + system_variant: 'songs_like_artist', + refreshable: false, + name: 'Songs like X' + }; + render(PlaylistCard, { props: { playlist: songsLike } }); + expect(screen.queryByLabelText(/playlist actions/i)).not.toBeInTheDocument(); + }); + + test('Refresh item appears in the kebab, labelled by name', async () => { const discoverPlaylist: Playlist = { ...base, kind: 'system', system_variant: 'discover', + refreshable: true, name: 'Discover' }; render(PlaylistCard, { props: { playlist: discoverPlaylist } }); - const kebabBtn = screen.getByLabelText(/playlist actions/i); - await fireEvent.click(kebabBtn); + await fireEvent.click(screen.getByLabelText(/playlist actions/i)); expect(screen.getByRole('menuitem', { name: /refresh discover/i })).toBeInTheDocument(); }); - test('Refresh For You item appears in For You kebab', async () => { - const forYouPlaylist: Playlist = { - ...base, - kind: 'system', - system_variant: 'for_you', - name: 'For You' - }; - render(PlaylistCard, { props: { playlist: forYouPlaylist } }); - const kebabBtn = screen.getByLabelText(/playlist actions/i); - await fireEvent.click(kebabBtn); - expect(screen.getByRole('menuitem', { name: /refresh for you/i })).toBeInTheDocument(); - }); - - test('clicking Refresh Discover calls refreshDiscover', async () => { - const { refreshDiscover } = await import('$lib/api/playlists'); + test('clicking Refresh calls refreshSystem with the raw variant', async () => { + const { refreshSystem } = await import('$lib/api/playlists'); const discoverPlaylist: Playlist = { ...base, kind: 'system', system_variant: 'discover', + refreshable: true, name: 'Discover' }; render(PlaylistCard, { props: { playlist: discoverPlaylist } }); - const kebabBtn = screen.getByLabelText(/playlist actions/i); - await fireEvent.click(kebabBtn); - const refreshItem = screen.getByRole('menuitem', { name: /refresh discover/i }); - await fireEvent.click(refreshItem); - await waitFor(() => expect(refreshDiscover).toHaveBeenCalled()); + await fireEvent.click(screen.getByLabelText(/playlist actions/i)); + await fireEvent.click(screen.getByRole('menuitem', { name: /refresh discover/i })); + await waitFor(() => expect(refreshSystem).toHaveBeenCalledWith('discover')); }); - test('clicking Refresh For You calls refreshForYou', async () => { - const { refreshForYou } = await import('$lib/api/playlists'); - const forYouPlaylist: Playlist = { - ...base, - kind: 'system', - system_variant: 'for_you', - name: 'For You' - }; - render(PlaylistCard, { props: { playlist: forYouPlaylist } }); - const kebabBtn = screen.getByLabelText(/playlist actions/i); - await fireEvent.click(kebabBtn); - const refreshItem = screen.getByRole('menuitem', { name: /refresh for you/i }); - await fireEvent.click(refreshItem); - await waitFor(() => expect(refreshForYou).toHaveBeenCalled()); - }); - - test('For-You play calls systemShuffle, not getPlaylist or refreshForYou', async () => { - const { refreshForYou, getPlaylist, systemShuffle } = await import('$lib/api/playlists'); + test('For-You play calls systemShuffle, not getPlaylist', async () => { + const { getPlaylist, systemShuffle } = await import('$lib/api/playlists'); const { playQueue } = await import('$lib/player/store.svelte'); const forYouPlaylist: Playlist = { ...base, kind: 'system', system_variant: 'for_you', + refreshable: true, name: 'For You', track_count: 25 }; @@ -274,11 +251,10 @@ describe('PlaylistCard', () => { await fireEvent.click(playButton); await new Promise(resolve => setTimeout(resolve, 10)); - // #415: system play hits the rotation-aware shuffle endpoint, - // not the cached detail GET, and never refreshes on press. + // #415/#411: system play hits the rotation-aware shuffle endpoint + // (raw variant), not the cached detail GET, and never refreshes. expect(systemShuffle).toHaveBeenCalledWith('for_you'); expect(getPlaylist).not.toHaveBeenCalled(); - expect(refreshForYou).not.toHaveBeenCalled(); expect(playQueue).toHaveBeenCalled(); }); @@ -315,8 +291,8 @@ describe('PlaylistCard', () => { expect(playQueue).toHaveBeenCalledWith(expect.any(Array), 0); }); - test('non-For-You play button does NOT call refreshForYou', async () => { - const { refreshForYou, getPlaylist } = await import('$lib/api/playlists'); + test('user-playlist play does not refresh anything', async () => { + const { refreshSystem, getPlaylist } = await import('$lib/api/playlists'); const { playQueue } = await import('$lib/player/store.svelte'); render(PlaylistCard, { props: { playlist: base } }); @@ -324,7 +300,7 @@ describe('PlaylistCard', () => { await fireEvent.click(playButton); await new Promise(resolve => setTimeout(resolve, 10)); - expect(refreshForYou).not.toHaveBeenCalled(); + expect(refreshSystem).not.toHaveBeenCalled(); expect(getPlaylist).toHaveBeenCalledWith('p-1'); expect(playQueue).toHaveBeenCalled(); }); diff --git a/web/src/routes/page.test.ts b/web/src/routes/page.test.ts index 884dd067..a4d6b76f 100644 --- a/web/src/routes/page.test.ts +++ b/web/src/routes/page.test.ts @@ -84,6 +84,7 @@ describe('home Playlists section', () => { is_public: false, kind: 'system', system_variant: 'for_you', + refreshable: false, seed_artist_id: null, cover_url: '', track_count: 25, diff --git a/web/src/routes/playlists/[id]/+page.svelte b/web/src/routes/playlists/[id]/+page.svelte index 8182ad39..0767e0f4 100644 --- a/web/src/routes/playlists/[id]/+page.svelte +++ b/web/src/routes/playlists/[id]/+page.svelte @@ -12,7 +12,7 @@ deletePlaylist, removePlaylistTrack, reorderPlaylist, - refreshDiscover + refreshSystem } from '$lib/api/playlists'; import { qk } from '$lib/api/queries'; import { user } from '$lib/auth/store.svelte'; @@ -130,20 +130,20 @@ } } - // --- Discover refresh --- - let refreshingDiscover = $state(false); + // --- System-playlist refresh (#411 R2: generic by-kind) --- + let refreshingSystem = $state(false); - async function onRefreshDiscover() { - refreshingDiscover = true; + async function onRefreshSystem(variant: string) { + refreshingSystem = true; try { - await refreshDiscover(); - pushToast('Discover refreshed.'); + await refreshSystem(variant); + pushToast('Refreshed.'); await queryClient.invalidateQueries({ queryKey: qk.playlist(id) }); await queryClient.invalidateQueries({ queryKey: qk.playlists() }); } catch (e: unknown) { pushToast(`Refresh failed: ${errCode(e)}`, 'error'); } finally { - refreshingDiscover = false; + refreshingSystem = false; } } @@ -177,15 +177,15 @@ {#if !isOwner}· by {pl.owner_username}{/if}

- {#if pl.system_variant === 'discover'} + {#if pl.refreshable && pl.system_variant} {/if} {#if isOwner} diff --git a/web/src/routes/playlists/[id]/playlist.test.ts b/web/src/routes/playlists/[id]/playlist.test.ts index 4eac7aef..031d170c 100644 --- a/web/src/routes/playlists/[id]/playlist.test.ts +++ b/web/src/routes/playlists/[id]/playlist.test.ts @@ -19,7 +19,7 @@ vi.mock('$lib/api/playlists', () => ({ deletePlaylist: vi.fn().mockResolvedValue(undefined), removePlaylistTrack: vi.fn().mockResolvedValue(undefined), reorderPlaylist: vi.fn().mockResolvedValue(undefined), - refreshDiscover: vi.fn().mockResolvedValue({ playlist_id: 'p1', track_count: 5 }) + refreshSystem: vi.fn().mockResolvedValue({ playlist_id: 'p1', track_count: 5, track_ids: [] }) })); vi.mock('$lib/auth/store.svelte', () => ({ @@ -44,13 +44,13 @@ vi.mock('$lib/api/quarantine', () => emptyQuarantineMock()); vi.mock('$lib/api/admin/tracks', () => ({ removeTrack: vi.fn() })); import PlaylistDetailPage from './+page.svelte'; -import { createPlaylistQuery, deletePlaylist, refreshDiscover } from '$lib/api/playlists'; +import { createPlaylistQuery, deletePlaylist, refreshSystem } from '$lib/api/playlists'; const mockedQuery = createPlaylistQuery as ReturnType; const ownDetail: PlaylistDetail = { id: 'p1', user_id: 'u-self', owner_username: 'me', name: 'Mine', - description: '', is_public: false, kind: 'user', system_variant: null, seed_artist_id: null, cover_url: '', track_count: 2, duration_sec: 274, + description: '', is_public: false, kind: 'user', system_variant: null, refreshable: false, seed_artist_id: null, cover_url: '', track_count: 2, duration_sec: 274, created_at: '', updated_at: '', tracks: [ { position: 0, track_id: 't1', album_id: 'a1', artist_id: 'ar1', title: 'A', artist_name: 'X', album_title: 'Y', duration_sec: 137, stream_url: '/api/tracks/t1/stream', added_at: '' }, @@ -87,43 +87,58 @@ describe('Playlist detail page', () => { confirmSpy.mockRestore(); }); - test('renders Refresh button only on Discover playlists', () => { + test('renders Refresh button on a refreshable system playlist', () => { const discoverDetail: PlaylistDetail = { ...ownDetail, kind: 'system', - system_variant: 'discover' + system_variant: 'discover', + refreshable: true }; mockedQuery.mockReturnValue(mockQuery({ data: discoverDetail })); render(PlaylistDetailPage); - expect(screen.getByLabelText(/refresh discover/i)).toBeInTheDocument(); + expect(screen.getByLabelText(/refresh playlist/i)).toBeInTheDocument(); }); - test('does not render Refresh button on for_you playlists', () => { + test('renders Refresh button on For You too (also refreshable now)', () => { const forYouDetail: PlaylistDetail = { ...ownDetail, kind: 'system', - system_variant: 'for_you' + system_variant: 'for_you', + refreshable: true }; mockedQuery.mockReturnValue(mockQuery({ data: forYouDetail })); render(PlaylistDetailPage); - expect(screen.queryByLabelText(/refresh discover/i)).not.toBeInTheDocument(); + expect(screen.getByLabelText(/refresh playlist/i)).toBeInTheDocument(); + }); + + test('does not render Refresh on a non-refreshable system playlist', () => { + const songsLike: PlaylistDetail = { + ...ownDetail, + kind: 'system', + system_variant: 'songs_like_artist', + refreshable: false + }; + mockedQuery.mockReturnValue(mockQuery({ data: songsLike })); + render(PlaylistDetailPage); + expect(screen.queryByLabelText(/refresh playlist/i)).not.toBeInTheDocument(); }); test('does not render Refresh button on user playlists', () => { mockedQuery.mockReturnValue(mockQuery({ data: ownDetail })); render(PlaylistDetailPage); - expect(screen.queryByLabelText(/refresh discover/i)).not.toBeInTheDocument(); + expect(screen.queryByLabelText(/refresh playlist/i)).not.toBeInTheDocument(); }); - test('Refresh button calls refreshDiscover when clicked', async () => { + test('Refresh button calls refreshSystem with the variant', async () => { const discoverDetail: PlaylistDetail = { ...ownDetail, kind: 'system', - system_variant: 'discover' + system_variant: 'discover', + refreshable: true }; mockedQuery.mockReturnValue(mockQuery({ data: discoverDetail })); render(PlaylistDetailPage); - await fireEvent.click(screen.getByLabelText(/refresh discover/i)); - await waitFor(() => expect(refreshDiscover).toHaveBeenCalled()); + await fireEvent.click(screen.getByLabelText(/refresh playlist/i)); + await waitFor(() => expect(refreshSystem).toHaveBeenCalledWith('discover')); }); }); diff --git a/web/src/routes/playlists/playlists.test.ts b/web/src/routes/playlists/playlists.test.ts index e54247ee..a9fe445e 100644 --- a/web/src/routes/playlists/playlists.test.ts +++ b/web/src/routes/playlists/playlists.test.ts @@ -19,7 +19,7 @@ const mockedCreatePlaylist = createPlaylist as ReturnType; function p(over: Partial): Playlist { return { id: 'p1', user_id: 'u-self', owner_username: 'me', name: 'A', - description: '', is_public: false, kind: 'user', system_variant: null, seed_artist_id: null, cover_url: '', track_count: 0, duration_sec: 0, + description: '', is_public: false, kind: 'user', system_variant: null, refreshable: false, seed_artist_id: null, cover_url: '', track_count: 0, duration_sec: 0, created_at: '', updated_at: '', ...over }; } From 222742e368a0c6c192906534767462131b283a8e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 13:29:27 -0400 Subject: [PATCH 16/27] test(api): replace per-kind refresh tests with generic system tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit go vet broke because playlists_{discover,foryou}_refresh_test.go referenced the handlers/types deleted in R2 (d67c0de). Consolidated into playlists_system_test.go covering the generic /playlists/system/{kind}/{refresh} endpoint: for_you + discover 200/shape, non-singleton & unknown kind → 404, no-auth → 401. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../api/playlists_discover_refresh_test.go | 64 ---------- internal/api/playlists_foryou_refresh_test.go | 67 ----------- internal/api/playlists_system_test.go | 111 ++++++++++++++++++ 3 files changed, 111 insertions(+), 131 deletions(-) delete mode 100644 internal/api/playlists_discover_refresh_test.go delete mode 100644 internal/api/playlists_foryou_refresh_test.go create mode 100644 internal/api/playlists_system_test.go diff --git a/internal/api/playlists_discover_refresh_test.go b/internal/api/playlists_discover_refresh_test.go deleted file mode 100644 index 0134c622..00000000 --- a/internal/api/playlists_discover_refresh_test.go +++ /dev/null @@ -1,64 +0,0 @@ -package api - -import ( - "encoding/json" - "net/http" - "net/http/httptest" - "os" - "testing" - - "github.com/go-chi/chi/v5" - - "git.fabledsword.com/bvandeusen/minstrel/internal/auth" -) - -func newDiscoverRefreshRouter(h *handlers) chi.Router { - r := chi.NewRouter() - r.Route("/api", func(api chi.Router) { - api.Use(auth.RequireUser(h.pool)) - api.Post("/playlists/system/discover/refresh", h.handleDiscoverRefresh) - }) - return r -} - -func TestDiscoverRefresh_AuthenticatedUser_Returns200(t *testing.T) { - if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { - t.Skip("MINSTREL_TEST_DATABASE_URL not set") - } - h, pool := testHandlers(t) - user := seedUser(t, pool, "discrefresh", "pw", false) - - req := httptest.NewRequest(http.MethodPost, "/api/playlists/system/discover/refresh", nil) - req = withUser(req, user) - rec := httptest.NewRecorder() - newDiscoverRefreshRouter(h).ServeHTTP(rec, req) - - if rec.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) - } - var resp discoverRefreshResp - if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { - t.Fatalf("decode: %v", err) - } - // On a fresh user with no library, the build runs and emits a - // Discover playlist (or no row if library is truly empty). - // Either case is OK; we only assert the response shape decoded. - if resp.TrackCount < 0 { - t.Errorf("track_count = %d, want >= 0", resp.TrackCount) - } -} - -func TestDiscoverRefresh_NoAuth_Returns401(t *testing.T) { - if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { - t.Skip("MINSTREL_TEST_DATABASE_URL not set") - } - h, _ := testHandlers(t) - - req := httptest.NewRequest(http.MethodPost, "/api/playlists/system/discover/refresh", nil) - rec := httptest.NewRecorder() - newDiscoverRefreshRouter(h).ServeHTTP(rec, req) - - if rec.Code != http.StatusUnauthorized { - t.Errorf("status = %d, want 401", rec.Code) - } -} diff --git a/internal/api/playlists_foryou_refresh_test.go b/internal/api/playlists_foryou_refresh_test.go deleted file mode 100644 index 76b6122a..00000000 --- a/internal/api/playlists_foryou_refresh_test.go +++ /dev/null @@ -1,67 +0,0 @@ -package api - -import ( - "encoding/json" - "net/http" - "net/http/httptest" - "os" - "testing" - - "github.com/go-chi/chi/v5" - - "git.fabledsword.com/bvandeusen/minstrel/internal/auth" -) - -func newForYouRefreshRouter(h *handlers) chi.Router { - r := chi.NewRouter() - r.Route("/api", func(api chi.Router) { - api.Use(auth.RequireUser(h.pool)) - api.Post("/playlists/system/for-you/refresh", h.handleForYouRefresh) - }) - return r -} - -func TestForYouRefresh_AuthenticatedUser_Returns200(t *testing.T) { - if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { - t.Skip("MINSTREL_TEST_DATABASE_URL not set") - } - h, pool := testHandlers(t) - user := seedUser(t, pool, "foryourefresh", "pw", false) - - req := httptest.NewRequest(http.MethodPost, "/api/playlists/system/for-you/refresh", nil) - req = withUser(req, user) - rec := httptest.NewRecorder() - newForYouRefreshRouter(h).ServeHTTP(rec, req) - - if rec.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) - } - var resp foryouRefreshResp - if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { - t.Fatalf("decode: %v", err) - } - if resp.TrackIDs == nil { - t.Errorf("track_ids = nil, want []string (empty array, not null)") - } - if int(resp.TrackCount) != len(resp.TrackIDs) { - // Acceptable mismatch only when some tracks were removed from - // the library between build and response. For a fresh test - // user the counts should match. - t.Errorf("track_count=%d, len(track_ids)=%d — mismatch", resp.TrackCount, len(resp.TrackIDs)) - } -} - -func TestForYouRefresh_NoAuth_Returns401(t *testing.T) { - if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { - t.Skip("MINSTREL_TEST_DATABASE_URL not set") - } - h, _ := testHandlers(t) - - req := httptest.NewRequest(http.MethodPost, "/api/playlists/system/for-you/refresh", nil) - rec := httptest.NewRecorder() - newForYouRefreshRouter(h).ServeHTTP(rec, req) - - if rec.Code != http.StatusUnauthorized { - t.Errorf("status = %d, want 401", rec.Code) - } -} diff --git a/internal/api/playlists_system_test.go b/internal/api/playlists_system_test.go new file mode 100644 index 00000000..1a1570e1 --- /dev/null +++ b/internal/api/playlists_system_test.go @@ -0,0 +1,111 @@ +package api + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "testing" + + "github.com/go-chi/chi/v5" + + "git.fabledsword.com/bvandeusen/minstrel/internal/auth" +) + +// Generic registry-driven system-playlist endpoints (#411 R2). +// Replaces the former per-kind refresh handler tests. +func newSystemPlaylistRouter(h *handlers) chi.Router { + r := chi.NewRouter() + r.Route("/api", func(api chi.Router) { + api.Use(auth.RequireUser(h.pool)) + api.Post("/playlists/system/{kind}/refresh", h.handleSystemPlaylistRefresh) + api.Get("/playlists/system/{kind}/shuffle", h.handleSystemPlaylistShuffle) + }) + return r +} + +func TestSystemRefresh_ForYou_Returns200(t *testing.T) { + if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + h, pool := testHandlers(t) + user := seedUser(t, pool, "sysforyou", "pw", false) + + req := httptest.NewRequest(http.MethodPost, "/api/playlists/system/for_you/refresh", nil) + req = withUser(req, user) + rec := httptest.NewRecorder() + newSystemPlaylistRouter(h).ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var resp systemRefreshResp + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if resp.TrackIDs == nil { + t.Errorf("track_ids = nil, want [] (empty array, not null)") + } + if int(resp.TrackCount) != len(resp.TrackIDs) { + t.Errorf("track_count=%d, len(track_ids)=%d — mismatch", resp.TrackCount, len(resp.TrackIDs)) + } +} + +func TestSystemRefresh_Discover_Returns200(t *testing.T) { + if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + h, pool := testHandlers(t) + user := seedUser(t, pool, "sysdiscover", "pw", false) + + req := httptest.NewRequest(http.MethodPost, "/api/playlists/system/discover/refresh", nil) + req = withUser(req, user) + rec := httptest.NewRecorder() + newSystemPlaylistRouter(h).ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var resp systemRefreshResp + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if resp.TrackCount < 0 { + t.Errorf("track_count = %d, want >= 0", resp.TrackCount) + } +} + +// A non-singleton (songs_like_artist) or unknown kind isn't +// addressable by the generic endpoints → 404, not a build. +func TestSystemRefresh_NonSingletonKind_Returns404(t *testing.T) { + if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + h, pool := testHandlers(t) + user := seedUser(t, pool, "sysbadkind", "pw", false) + + for _, kind := range []string{"songs_like_artist", "bogus"} { + req := httptest.NewRequest(http.MethodPost, "/api/playlists/system/"+kind+"/refresh", nil) + req = withUser(req, user) + rec := httptest.NewRecorder() + newSystemPlaylistRouter(h).ServeHTTP(rec, req) + if rec.Code != http.StatusNotFound { + t.Errorf("kind=%s: status = %d, want 404", kind, rec.Code) + } + } +} + +func TestSystemRefresh_NoAuth_Returns401(t *testing.T) { + if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + h, _ := testHandlers(t) + + req := httptest.NewRequest(http.MethodPost, "/api/playlists/system/for_you/refresh", nil) + rec := httptest.NewRecorder() + newSystemPlaylistRouter(h).ServeHTTP(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Errorf("status = %d, want 401", rec.Code) + } +} From b9accf6934cc81489e8eeae608b164a4a8434ecc Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 13:33:52 -0400 Subject: [PATCH 17/27] style: gofmt playlists.go after Refreshable field add (#411 R2) The commented Refreshable field broke gofmt's struct-tag column alignment in playlistRowView. Pure formatting. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/playlists.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/api/playlists.go b/internal/api/playlists.go index bd2779a0..e74291d3 100644 --- a/internal/api/playlists.go +++ b/internal/api/playlists.go @@ -38,11 +38,11 @@ type playlistRowView struct { // which kinds support it (false for user + songs_like_artist). Refreshable bool `json:"refreshable"` SeedArtistID *string `json:"seed_artist_id,omitempty"` - CoverURL string `json:"cover_url,omitempty"` - TrackCount int32 `json:"track_count"` - DurationSec int32 `json:"duration_sec"` - CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` + CoverURL string `json:"cover_url,omitempty"` + TrackCount int32 `json:"track_count"` + DurationSec int32 `json:"duration_sec"` + CreatedAt string `json:"created_at"` + UpdatedAt string `json:"updated_at"` } // playlistTrackView is one row of a playlist's track list on the wire. From c7ee0871a5f9a3ef20a567dbb6f406e4a4a9b6e4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 13:55:27 -0400 Subject: [PATCH 18/27] =?UTF-8?q?feat(playlists):=20#411=20R3=20=E2=80=94?= =?UTF-8?q?=20five=20discovery=20mixes=20(#419-423)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each is one candidate query + one registry entry; zero client work (R2 made tiles/refresh/shuffle generic, all are singleton kinds so web's server `refreshable` flag and Flutter's derived getter both light up automatically). - deep_cuts (#419): <=2-play tracks from liked / heavily-played artists; diversity-capped. - rediscover (#420): >=5-play tracks not heard in 6 months, by historical affection. - new_for_you (#421): tracks from albums added <=30d whose artist the user likes/plays; album-coherent (no cap). - on_this_day (#422): tracks played within ±7 day-of-year in prior windows (>60d ago), weighted by play count. - first_listens (#423): never-played albums, tiered liked-artist → played-artist → rest; album-coherent. system_mixes.go producers mirror the Discover model (SQL gives the ranking; finishMix caps+truncates to 100 to match For-You/Discover shuffle depth; album-coherent mixes skip the cap). Builder query failure is non-fatal (logged, yields no playlist) like Discover. Existing system_test existence checks are unaffected. Closes the #411 system-playlists-v2 umbrella's new-types thread. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/db/dbq/system_mixes.sql.go | 303 +++++++++++++++++++++++++++ internal/db/queries/system_mixes.sql | 145 +++++++++++++ internal/playlists/system.go | 5 + internal/playlists/system_mixes.go | 145 +++++++++++++ 4 files changed, 598 insertions(+) create mode 100644 internal/db/dbq/system_mixes.sql.go create mode 100644 internal/db/queries/system_mixes.sql create mode 100644 internal/playlists/system_mixes.go diff --git a/internal/db/dbq/system_mixes.sql.go b/internal/db/dbq/system_mixes.sql.go new file mode 100644 index 00000000..5557b28b --- /dev/null +++ b/internal/db/dbq/system_mixes.sql.go @@ -0,0 +1,303 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: system_mixes.sql + +package dbq + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const listDeepCutsTracks = `-- name: ListDeepCutsTracks :many + +WITH affinity_artists AS ( + SELECT artist_id FROM general_likes_artists WHERE user_id = $1 + UNION + SELECT t.artist_id + FROM play_events pe JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 AND pe.was_skipped = false + GROUP BY t.artist_id + HAVING COUNT(*) >= 5 +), +play_counts AS ( + SELECT track_id, COUNT(*) AS c + FROM play_events + WHERE user_id = $1 AND was_skipped = false + GROUP BY track_id +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN affinity_artists aa ON aa.artist_id = t.artist_id + LEFT JOIN play_counts pc ON pc.track_id = t.id + WHERE COALESCE(pc.c, 0) <= 2 + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY md5(t.id::text || $2::text) + LIMIT 200 +` + +type ListDeepCutsTracksParams struct { + UserID pgtype.UUID + Column2 string +} + +type ListDeepCutsTracksRow struct { + ID pgtype.UUID + AlbumID pgtype.UUID + ArtistID pgtype.UUID +} + +// Candidate queries for the discovery system-playlist mixes +// (#419-423). Each returns (id, album_id, artist_id) rows; the Go +// producer caps + truncates and converts to rankedCandidate. Daily- +// deterministic md5(id||dateStr) ordering (where used) keeps a mix +// stable within a day while rotating across days; shuffle-on-play +// gives per-play variety on top. +// #419 Deep Cuts: low-play tracks (<=2 plays) from artists the user +// has liked OR played heavily (>=5 non-skip plays across the artist). +// $1 user_id, $2 date string. +func (q *Queries) ListDeepCutsTracks(ctx context.Context, arg ListDeepCutsTracksParams) ([]ListDeepCutsTracksRow, error) { + rows, err := q.db.Query(ctx, listDeepCutsTracks, arg.UserID, arg.Column2) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListDeepCutsTracksRow + for rows.Next() { + var i ListDeepCutsTracksRow + if err := rows.Scan(&i.ID, &i.AlbumID, &i.ArtistID); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listFirstListensTracks = `-- name: ListFirstListensTracks :many +WITH heard_albums AS ( + SELECT DISTINCT t.album_id + FROM play_events pe JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 +), +played_artists AS ( + SELECT DISTINCT t.artist_id + FROM play_events pe JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN albums al ON al.id = t.album_id + WHERE NOT EXISTS (SELECT 1 FROM heard_albums h WHERE h.album_id = al.id) + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY + (CASE + WHEN EXISTS ( + SELECT 1 FROM general_likes_artists gla + WHERE gla.user_id = $1 AND gla.artist_id = al.artist_id + ) THEN 0 + WHEN al.artist_id IN (SELECT artist_id FROM played_artists) THEN 1 + ELSE 2 + END), + al.id, t.disc_number NULLS FIRST, t.track_number NULLS FIRST + LIMIT 300 +` + +type ListFirstListensTracksRow struct { + ID pgtype.UUID + AlbumID pgtype.UUID + ArtistID pgtype.UUID +} + +// #423 First Listens: albums the user has never played any track of. +// Tiered: liked-artist albums first, then played-artist albums, then +// the rest — album-coherent within each tier. Not diversity-capped +// (whole-album discovery). $1 user_id. +func (q *Queries) ListFirstListensTracks(ctx context.Context, userID pgtype.UUID) ([]ListFirstListensTracksRow, error) { + rows, err := q.db.Query(ctx, listFirstListensTracks, userID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListFirstListensTracksRow + for rows.Next() { + var i ListFirstListensTracksRow + if err := rows.Scan(&i.ID, &i.AlbumID, &i.ArtistID); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listNewForYouTracks = `-- name: ListNewForYouTracks :many +WITH affinity_artists AS ( + SELECT artist_id FROM general_likes_artists WHERE user_id = $1 + UNION + SELECT t.artist_id + FROM play_events pe JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 AND pe.was_skipped = false + GROUP BY t.artist_id + HAVING COUNT(*) >= 3 +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN albums al ON al.id = t.album_id + JOIN affinity_artists aa ON aa.artist_id = al.artist_id + WHERE al.created_at >= now() - interval '30 days' + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY al.created_at DESC, t.disc_number NULLS FIRST, t.track_number NULLS FIRST + LIMIT 200 +` + +type ListNewForYouTracksRow struct { + ID pgtype.UUID + AlbumID pgtype.UUID + ArtistID pgtype.UUID +} + +// #421 New for you: tracks from albums added in the last 30 days +// whose artist the user has liked OR played (>=3 non-skip). Album- +// coherent (newest album first, then disc/track) — the producer does +// NOT diversity-cap these; they're meant as whole-album discovery. +// $1 user_id. +func (q *Queries) ListNewForYouTracks(ctx context.Context, userID pgtype.UUID) ([]ListNewForYouTracksRow, error) { + rows, err := q.db.Query(ctx, listNewForYouTracks, userID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListNewForYouTracksRow + for rows.Next() { + var i ListNewForYouTracksRow + if err := rows.Scan(&i.ID, &i.AlbumID, &i.ArtistID); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listOnThisDayTracks = `-- name: ListOnThisDayTracks :many +WITH windowed AS ( + SELECT track_id, COUNT(*) AS c + FROM play_events + WHERE user_id = $1 AND was_skipped = false + AND started_at < now() - interval '60 days' + AND ABS(EXTRACT(DOY FROM started_at) - EXTRACT(DOY FROM now())) <= 7 + GROUP BY track_id +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN windowed w ON w.track_id = t.id + WHERE NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY w.c DESC, md5(t.id::text || $2::text) + LIMIT 200 +` + +type ListOnThisDayTracksParams struct { + UserID pgtype.UUID + Column2 string +} + +type ListOnThisDayTracksRow struct { + ID pgtype.UUID + AlbumID pgtype.UUID + ArtistID pgtype.UUID +} + +// #422 On This Day: tracks the user played around this calendar date +// (±7 day-of-year) in the past, excluding the very recent (>60 days +// ago) so it's nostalgic, not just "last week". Weighted by how much +// they were played in those windows. +// $1 user_id, $2 date string. +func (q *Queries) ListOnThisDayTracks(ctx context.Context, arg ListOnThisDayTracksParams) ([]ListOnThisDayTracksRow, error) { + rows, err := q.db.Query(ctx, listOnThisDayTracks, arg.UserID, arg.Column2) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListOnThisDayTracksRow + for rows.Next() { + var i ListOnThisDayTracksRow + if err := rows.Scan(&i.ID, &i.AlbumID, &i.ArtistID); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listRediscoverTracks = `-- name: ListRediscoverTracks :many +WITH stats AS ( + SELECT track_id, COUNT(*) AS c, MAX(started_at) AS last_at + FROM play_events + WHERE user_id = $1 AND was_skipped = false + GROUP BY track_id +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN stats s ON s.track_id = t.id + WHERE s.c >= 5 + AND s.last_at <= now() - interval '6 months' + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY s.c DESC, t.id + LIMIT 200 +` + +type ListRediscoverTracksRow struct { + ID pgtype.UUID + AlbumID pgtype.UUID + ArtistID pgtype.UUID +} + +// #420 Rediscover: tracks the user played a lot (>=5 non-skip) but +// not in the last 6 months. Ordered by historical affection. +// $1 user_id. +func (q *Queries) ListRediscoverTracks(ctx context.Context, userID pgtype.UUID) ([]ListRediscoverTracksRow, error) { + rows, err := q.db.Query(ctx, listRediscoverTracks, userID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListRediscoverTracksRow + for rows.Next() { + var i ListRediscoverTracksRow + if err := rows.Scan(&i.ID, &i.AlbumID, &i.ArtistID); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} diff --git a/internal/db/queries/system_mixes.sql b/internal/db/queries/system_mixes.sql new file mode 100644 index 00000000..62074620 --- /dev/null +++ b/internal/db/queries/system_mixes.sql @@ -0,0 +1,145 @@ +-- Candidate queries for the discovery system-playlist mixes +-- (#419-423). Each returns (id, album_id, artist_id) rows; the Go +-- producer caps + truncates and converts to rankedCandidate. Daily- +-- deterministic md5(id||dateStr) ordering (where used) keeps a mix +-- stable within a day while rotating across days; shuffle-on-play +-- gives per-play variety on top. + +-- name: ListDeepCutsTracks :many +-- #419 Deep Cuts: low-play tracks (<=2 plays) from artists the user +-- has liked OR played heavily (>=5 non-skip plays across the artist). +-- $1 user_id, $2 date string. +WITH affinity_artists AS ( + SELECT artist_id FROM general_likes_artists WHERE user_id = $1 + UNION + SELECT t.artist_id + FROM play_events pe JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 AND pe.was_skipped = false + GROUP BY t.artist_id + HAVING COUNT(*) >= 5 +), +play_counts AS ( + SELECT track_id, COUNT(*) AS c + FROM play_events + WHERE user_id = $1 AND was_skipped = false + GROUP BY track_id +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN affinity_artists aa ON aa.artist_id = t.artist_id + LEFT JOIN play_counts pc ON pc.track_id = t.id + WHERE COALESCE(pc.c, 0) <= 2 + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY md5(t.id::text || $2::text) + LIMIT 200; + +-- name: ListRediscoverTracks :many +-- #420 Rediscover: tracks the user played a lot (>=5 non-skip) but +-- not in the last 6 months. Ordered by historical affection. +-- $1 user_id. +WITH stats AS ( + SELECT track_id, COUNT(*) AS c, MAX(started_at) AS last_at + FROM play_events + WHERE user_id = $1 AND was_skipped = false + GROUP BY track_id +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN stats s ON s.track_id = t.id + WHERE s.c >= 5 + AND s.last_at <= now() - interval '6 months' + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY s.c DESC, t.id + LIMIT 200; + +-- name: ListNewForYouTracks :many +-- #421 New for you: tracks from albums added in the last 30 days +-- whose artist the user has liked OR played (>=3 non-skip). Album- +-- coherent (newest album first, then disc/track) — the producer does +-- NOT diversity-cap these; they're meant as whole-album discovery. +-- $1 user_id. +WITH affinity_artists AS ( + SELECT artist_id FROM general_likes_artists WHERE user_id = $1 + UNION + SELECT t.artist_id + FROM play_events pe JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 AND pe.was_skipped = false + GROUP BY t.artist_id + HAVING COUNT(*) >= 3 +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN albums al ON al.id = t.album_id + JOIN affinity_artists aa ON aa.artist_id = al.artist_id + WHERE al.created_at >= now() - interval '30 days' + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY al.created_at DESC, t.disc_number NULLS FIRST, t.track_number NULLS FIRST + LIMIT 200; + +-- name: ListOnThisDayTracks :many +-- #422 On This Day: tracks the user played around this calendar date +-- (±7 day-of-year) in the past, excluding the very recent (>60 days +-- ago) so it's nostalgic, not just "last week". Weighted by how much +-- they were played in those windows. +-- $1 user_id, $2 date string. +WITH windowed AS ( + SELECT track_id, COUNT(*) AS c + FROM play_events + WHERE user_id = $1 AND was_skipped = false + AND started_at < now() - interval '60 days' + AND ABS(EXTRACT(DOY FROM started_at) - EXTRACT(DOY FROM now())) <= 7 + GROUP BY track_id +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN windowed w ON w.track_id = t.id + WHERE NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY w.c DESC, md5(t.id::text || $2::text) + LIMIT 200; + +-- name: ListFirstListensTracks :many +-- #423 First Listens: albums the user has never played any track of. +-- Tiered: liked-artist albums first, then played-artist albums, then +-- the rest — album-coherent within each tier. Not diversity-capped +-- (whole-album discovery). $1 user_id. +WITH heard_albums AS ( + SELECT DISTINCT t.album_id + FROM play_events pe JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 +), +played_artists AS ( + SELECT DISTINCT t.artist_id + FROM play_events pe JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 +) +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN albums al ON al.id = t.album_id + WHERE NOT EXISTS (SELECT 1 FROM heard_albums h WHERE h.album_id = al.id) + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY + (CASE + WHEN EXISTS ( + SELECT 1 FROM general_likes_artists gla + WHERE gla.user_id = $1 AND gla.artist_id = al.artist_id + ) THEN 0 + WHEN al.artist_id IN (SELECT artist_id FROM played_artists) THEN 1 + ELSE 2 + END), + al.id, t.disc_number NULLS FIRST, t.track_number NULLS FIRST + LIMIT 300; diff --git a/internal/playlists/system.go b/internal/playlists/system.go index 97b97e8f..c0610dad 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -282,6 +282,11 @@ var systemPlaylistRegistry = []systemPlaylistKind{ {Key: "for_you", Singleton: true, Produce: produceForYou}, {Key: "songs_like_artist", Singleton: false, Produce: produceSeedMixes}, {Key: "discover", Singleton: true, Produce: produceDiscover}, + {Key: "deep_cuts", Singleton: true, Produce: produceDeepCuts}, + {Key: "rediscover", Singleton: true, Produce: produceRediscover}, + {Key: "new_for_you", Singleton: true, Produce: produceNewForYou}, + {Key: "on_this_day", Singleton: true, Produce: produceOnThisDay}, + {Key: "first_listens", Singleton: true, Produce: produceFirstListens}, } // produceForYou: today's seed from the user's top-5 played tracks diff --git a/internal/playlists/system_mixes.go b/internal/playlists/system_mixes.go new file mode 100644 index 00000000..e57233ea --- /dev/null +++ b/internal/playlists/system_mixes.go @@ -0,0 +1,145 @@ +package playlists + +import ( + "context" + "log/slog" + "time" + + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// Discovery mixes (#419-423). Each is one candidate query + a thin +// producer registered in systemPlaylistRegistry. They follow the +// Discover model: a SQL query returns ordered (id, album_id, +// artist_id) rows; we optionally diversity-cap, truncate, and emit +// rankedCandidate (Score unused — SQL gave the ranking). All are +// singleton kinds, so the generic by-kind refresh/shuffle endpoints +// and per-tile refresh affordance work with zero client changes. + +// discoveryMixLen caps each mix at the same depth as For-You / +// Discover so shuffle-on-play has a varied pool within a day. +const discoveryMixLen = 100 + +// finishMix caps (per-album<=2 / per-artist<=3) when diversify is +// set, truncates to discoveryMixLen, and converts to the insert +// type. Album-coherent mixes (New for you, First Listens) pass +// diversify=false so whole albums survive. +func finishMix(rows []discoverTrack, diversify bool) []rankedCandidate { + pool := rows + if diversify { + pool = capByAlbumAndArtist(pool) + } + if len(pool) > discoveryMixLen { + pool = pool[:discoveryMixLen] + } + if len(pool) == 0 { + return nil + } + tracks := make([]rankedCandidate, len(pool)) + for i, t := range pool { + tracks[i] = rankedCandidate{TrackID: t.ID} + } + return tracks +} + +// emit wraps the finished track list in a single builtPlaylist (the +// discovery mixes are all singletons). nil tracks → no playlist. +func emit(name, variant string, tracks []rankedCandidate) []builtPlaylist { + if len(tracks) == 0 { + return nil + } + return []builtPlaylist{{Name: name, Variant: variant, Tracks: tracks}} +} + +func produceDeepCuts( + ctx context.Context, q *dbq.Queries, logger *slog.Logger, + userID pgtype.UUID, dateStr string, _ time.Time, +) ([]builtPlaylist, error) { + rows, err := q.ListDeepCutsTracks(ctx, dbq.ListDeepCutsTracksParams{ + UserID: userID, Column2: dateStr, + }) + if err != nil { + logger.Warn("system playlist: deep-cuts query failed; skipping", + "user_id", uuidStringPL(userID), "err", err) + return nil, nil + } + dt := make([]discoverTrack, len(rows)) + for i, r := range rows { + dt[i] = discoverTrack{ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID} + } + return emit("Deep Cuts", "deep_cuts", finishMix(dt, true)), nil +} + +func produceRediscover( + ctx context.Context, q *dbq.Queries, logger *slog.Logger, + userID pgtype.UUID, _ string, _ time.Time, +) ([]builtPlaylist, error) { + rows, err := q.ListRediscoverTracks(ctx, userID) + if err != nil { + logger.Warn("system playlist: rediscover query failed; skipping", + "user_id", uuidStringPL(userID), "err", err) + return nil, nil + } + dt := make([]discoverTrack, len(rows)) + for i, r := range rows { + dt[i] = discoverTrack{ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID} + } + return emit("Rediscover", "rediscover", finishMix(dt, true)), nil +} + +func produceNewForYou( + ctx context.Context, q *dbq.Queries, logger *slog.Logger, + userID pgtype.UUID, _ string, _ time.Time, +) ([]builtPlaylist, error) { + rows, err := q.ListNewForYouTracks(ctx, userID) + if err != nil { + logger.Warn("system playlist: new-for-you query failed; skipping", + "user_id", uuidStringPL(userID), "err", err) + return nil, nil + } + dt := make([]discoverTrack, len(rows)) + for i, r := range rows { + dt[i] = discoverTrack{ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID} + } + // Album-coherent: no diversity cap so whole new albums survive. + return emit("New for you", "new_for_you", finishMix(dt, false)), nil +} + +func produceOnThisDay( + ctx context.Context, q *dbq.Queries, logger *slog.Logger, + userID pgtype.UUID, dateStr string, _ time.Time, +) ([]builtPlaylist, error) { + rows, err := q.ListOnThisDayTracks(ctx, dbq.ListOnThisDayTracksParams{ + UserID: userID, Column2: dateStr, + }) + if err != nil { + logger.Warn("system playlist: on-this-day query failed; skipping", + "user_id", uuidStringPL(userID), "err", err) + return nil, nil + } + dt := make([]discoverTrack, len(rows)) + for i, r := range rows { + dt[i] = discoverTrack{ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID} + } + return emit("On this day", "on_this_day", finishMix(dt, true)), nil +} + +func produceFirstListens( + ctx context.Context, q *dbq.Queries, logger *slog.Logger, + userID pgtype.UUID, _ string, _ time.Time, +) ([]builtPlaylist, error) { + rows, err := q.ListFirstListensTracks(ctx, userID) + if err != nil { + logger.Warn("system playlist: first-listens query failed; skipping", + "user_id", uuidStringPL(userID), "err", err) + return nil, nil + } + dt := make([]discoverTrack, len(rows)) + for i, r := range rows { + dt[i] = discoverTrack{ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID} + } + // Album-coherent (tiered by liked/played artist in SQL): no cap. + return emit("First listens", "first_listens", finishMix(dt, false)), nil +} From a0aea00667e71b5a1c66d0795b00d0259969fec1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 16:13:55 -0400 Subject: [PATCH 19/27] =?UTF-8?q?feat(offline):=20#427=20S1=20=E2=80=94=20?= =?UTF-8?q?reachability=20offline=20marker?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit offlineProvider: a Notifier driven by a periodic /healthz probe. Offline after N=3 consecutive failed probes; recovers on the first success. Slow heartbeat when online (30s), faster when offline (10s) so recovery is noticed quickly. 5s initial delay for provider warmup; optimistic (false) until proven otherwise. Deliberately NOT coupled to connectivityProvider — subscribing to that StreamProvider eagerly mounts its 2s timeout and leaks a pending Timer through widget tests (the MutationReplayer bug). /healthz failing already covers interface-down. No .timeout() wrapper either (dio's own timeouts bound the probe) so the only Timers are the tracked initial+periodic, both cancelled via ref.onDispose — the proven smoke-safe shape. Wired in app.dart postFrame to start the poller. No UI yet; S4 gates system-playlist play + Shuffle-all on it. Co-Authored-By: Claude Opus 4.7 (1M context) --- flutter_client/lib/app.dart | 5 + .../lib/cache/offline_provider.dart | 105 ++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 flutter_client/lib/cache/offline_provider.dart diff --git a/flutter_client/lib/app.dart b/flutter_client/lib/app.dart index d4a1150e..93a6684c 100644 --- a/flutter_client/lib/app.dart +++ b/flutter_client/lib/app.dart @@ -3,6 +3,7 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'cache/cache_filler.dart'; import 'cache/metadata_prefetcher.dart'; +import 'cache/offline_provider.dart'; import 'cache/mutation_queue.dart'; import 'cache/prefetcher.dart'; import 'cache/sync_controller.dart'; @@ -61,6 +62,10 @@ class _MinstrelAppState extends ConsumerState { // scoring, scrobbles, and system-playlist rotation, and is the // path that carries the `source` tag for #415. ref.read(playEventsReporterProvider); + // Offline marker (#427 S1): periodic /healthz reachability + // probe → offlineProvider. Read here to start the poller; S4 + // gates system-playlist play + Shuffle-all on it. + ref.read(offlineProvider); }); } diff --git a/flutter_client/lib/cache/offline_provider.dart b/flutter_client/lib/cache/offline_provider.dart new file mode 100644 index 00000000..0ef4bcc4 --- /dev/null +++ b/flutter_client/lib/cache/offline_provider.dart @@ -0,0 +1,105 @@ +// Reachability-based offline marker (#427 S1). +// +// `connectivityProvider` only knows whether a network *interface* is +// up — not whether the Minstrel server is actually reachable (captive +// portals, server down, DNS, VPN-only routes all read "online"). This +// is the single source of truth other features gate on: offline = +// the server failed its /healthz probe N times in a row; recovery on +// the first success. +// +// Deliberately NOT coupled to connectivityProvider: subscribing to +// that StreamProvider eagerly mounts its 2s checkConnectivity timeout +// and leaks a pending Timer through widget tests that never reach the +// auth state (the bug fixed for MutationReplayer). /healthz failing +// already covers interface-down — it just fails the probe. +// +// Optimistic: assume online until proven offline, so a cold launch +// behaves normally and only flips after sustained unreachability. + +import 'dart:async'; + +import 'package:dio/dio.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; + +import '../api/endpoints/health.dart'; +import '../library/library_providers.dart' show dioProvider; + +class OfflineMonitor extends Notifier { + Timer? _initial; + Timer? _periodic; + int _consecutiveFails = 0; + + /// Consecutive failed probes before we declare offline. Small + /// enough to react within ~half a minute, large enough that a + /// single dropped request doesn't flip the whole UI. + static const _threshold = 3; + + /// Probe cadence. Slower when believed-online (cheap heartbeat); + /// faster when offline so recovery is noticed quickly. + static const _onlineInterval = Duration(seconds: 30); + static const _offlineInterval = Duration(seconds: 10); + + /// Let auth + server-url + dio providers finish async warmup + /// before the first probe so a cold start doesn't false-positive. + static const _initialDelay = Duration(seconds: 5); + + @override + bool build() { + ref.onDispose(() { + _initial?.cancel(); + _periodic?.cancel(); + }); + _initial = Timer(_initialDelay, () { + _check(); + _arm(); + }); + return false; // optimistic until a probe says otherwise + } + + void _arm() { + _periodic?.cancel(); + _periodic = Timer.periodic( + state ? _offlineInterval : _onlineInterval, + (_) => _check(), + ); + } + + Future _check() async { + final Dio dio; + try { + // Not configured yet (no server URL) → don't flip; the app is + // still on the connect screen and "offline" is meaningless. + dio = await ref.read(dioProvider.future); + } catch (_) { + return; + } + try { + // dio's own connect/receive timeouts bound this — no extra + // Timer (which would leak in widget tests). + await HealthApi(dio).check(); + _consecutiveFails = 0; + _set(false); + } catch (_) { + _consecutiveFails++; + if (_consecutiveFails >= _threshold) { + _set(true); + } + } + } + + void _set(bool offline) { + if (state == offline) return; + state = offline; + _arm(); // cadence follows the new state + } + + /// Force an immediate probe — e.g. a user-initiated retry. Public + /// so S4's offline surfaces can offer a "try again" affordance. + Future recheck() => _check(); +} + +/// `true` when the server is unreachable. Watch this to gate +/// online-only affordances; read it once at app start to activate +/// the poller. +final offlineProvider = + NotifierProvider(OfflineMonitor.new); From 3f61079c029eb8a5bbc29146e1e37dcf45c5146c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 21:07:45 -0400 Subject: [PATCH 20/27] =?UTF-8?q?feat(offline):=20#427=20S2(+S3)=20?= =?UTF-8?q?=E2=80=94=20two-bucket=20cache=20model?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the single 5GB capBytes with independent Liked + Rolling budgets (5GB each default). Bucket = liked-ness, NOT CacheSource: a cached track currently in the liked set is charged to / evicted under Liked; everything else is Rolling. Storage-only dedup — it never filters playback (S4's offline lists query the whole index). - db.dart: schema 8→9, AudioCacheIndex.lastPlayedAt (real play recency for S4 + rolling LRU; migration backfills to cachedAt). drift codegen run. - cache_settings: likedCapBytes + rollingCapBytes (+ setters); old cache_cap_bytes key dropped, defaults reapply (not data loss). - audio_cache_manager: touch(); bucketUsage() counts orphan partials (LockCaching files never indexed) as Rolling so the cap truly bounds disk; evictBuckets() drains non-liked LRU then sweeps orphans, Liked only by its own (large) cap — normal use never evicts the user's liked library. - prefetcher → evictBuckets with the cached liked set. - storage_section: two cap selectors + per-bucket usage (folds in S3 to avoid a broken intermediate). - Explicit Download dropped: removed album + playlist Download buttons, autoPlaylist pins, now-unused imports. - Tests updated/compiled (drift-cohort tests are CI-skipped). High blast radius (eviction deletes files) — liked-protective by design; needs operator device-check before "done". Co-Authored-By: Claude Opus 4.7 (1M context) --- .../lib/cache/audio_cache_manager.dart | 213 +++++++++++++----- .../lib/cache/cache_settings_provider.dart | 57 +++-- flutter_client/lib/cache/db.dart | 18 +- flutter_client/lib/cache/prefetcher.dart | 16 +- .../lib/library/album_detail_screen.dart | 17 -- .../lib/playlists/playlist_detail_screen.dart | 26 +-- .../lib/settings/storage_section.dart | 118 +++++++--- .../test/cache/audio_cache_manager_test.dart | 53 +++-- .../cache/cache_settings_provider_test.dart | 4 +- .../test/settings/storage_section_test.dart | 6 +- 10 files changed, 355 insertions(+), 173 deletions(-) diff --git a/flutter_client/lib/cache/audio_cache_manager.dart b/flutter_client/lib/cache/audio_cache_manager.dart index 5c4afecb..a26aa665 100644 --- a/flutter_client/lib/cache/audio_cache_manager.dart +++ b/flutter_client/lib/cache/audio_cache_manager.dart @@ -8,15 +8,20 @@ import 'package:path_provider/path_provider.dart'; import '../library/library_providers.dart' show dioProvider; import 'db.dart'; +/// Per-bucket cache usage (#427 S2). Rolling includes orphan files — +/// partials written by LockCachingAudioSource that were never indexed +/// (skip-before-fully-buffered) — so the rolling cap actually bounds +/// disk. +typedef BucketUsage = ({int liked, int rolling}); + /// Owns the audio cache directory + drift index. -/// API: -/// - isCached(trackId) -/// - pathFor(trackId) -/// - pin(trackId, source) — downloads + indexes -/// - unpin(trackId) -/// - evict(targetBytes) — tiered LRU -/// - usageBytes() -/// - clearAll() +/// +/// #427 S2: two storage buckets keyed by liked-ness, not by +/// CacheSource. A cached track currently in the user's liked set is +/// charged to (and evicted under) the Liked budget; everything else +/// is Rolling. This dedup is storage/eviction-only — it never filters +/// what the offline surfaces can play (S4). `CacheSource` is retained +/// on the API for callers but is now informational. class AudioCacheManager { AudioCacheManager({ required AppDb db, @@ -37,7 +42,6 @@ class AudioCacheManager { return d.path; } - /// True if the trackId has a complete file on disk. Future isCached(String trackId) async { final row = await (_db.select(_db.audioCacheIndex) ..where((t) => t.trackId.equals(trackId))) @@ -46,7 +50,6 @@ class AudioCacheManager { return File(row.path).existsSync(); } - /// Returns the local file path if cached, else null. Future pathFor(String trackId) async { final row = await (_db.select(_db.audioCacheIndex) ..where((t) => t.trackId.equals(trackId))) @@ -55,12 +58,14 @@ class AudioCacheManager { return File(row.path).existsSync() ? row.path : null; } - /// Downloads the track's stream to disk and indexes it. Idempotent — - /// returns the existing path if already cached. + /// Downloads the track's stream to disk and indexes it. Idempotent. + /// lastPlayedAt is set now — pinning is play-intent. Future pin(String trackId, {required CacheSource source}) async { final existing = await pathFor(trackId); - if (existing != null) return existing; - + if (existing != null) { + await touch(trackId); + return existing; + } final dir = await _tracksDir(); final path = '$dir/$trackId.mp3'; final dio = await _dioFactory(); @@ -78,16 +83,15 @@ class AudioCacheManager { path: path, sizeBytes: size, source: source, + lastPlayedAt: Value(DateTime.now()), ), ); return path; } - /// Registers a file already on disk in the cache index. Intended for - /// the streaming path (LockCachingAudioSource) which writes the file - /// itself; we need an index row so eviction can find and delete it - /// when usage exceeds the cap. `source` defaults to incidental so - /// stream-cached tracks are first to be evicted. + /// Registers a file LockCachingAudioSource wrote itself. Called + /// once a track is fully buffered; it's being played, so stamp + /// lastPlayedAt. Future registerStreamCache( String trackId, String path, @@ -100,11 +104,20 @@ class AudioCacheManager { path: path, sizeBytes: sizeBytes, source: source, + lastPlayedAt: Value(DateTime.now()), ), ); } - /// Removes a track from the index AND deletes the file. + /// Bumps lastPlayedAt for an already-indexed track so the rolling + /// LRU + the offline "Recently played" view reflect real plays + /// (not just download time). No-op if not indexed. + Future touch(String trackId) async { + await (_db.update(_db.audioCacheIndex) + ..where((t) => t.trackId.equals(trackId))) + .write(AudioCacheIndexCompanion(lastPlayedAt: Value(DateTime.now()))); + } + Future unpin(String trackId) async { final row = await (_db.select(_db.audioCacheIndex) ..where((t) => t.trackId.equals(trackId))) @@ -117,66 +130,150 @@ class AudioCacheManager { .go(); } - /// Total bytes used by the cache. Walks the cache directory directly - /// instead of summing the index, because the streaming-as-you-play - /// path (audio_handler's LockCachingAudioSource) writes files - /// without registering an index row. The index sum would always - /// understate (often to zero) for users who only stream. + /// Total bytes on disk (directory walk — authoritative; catches + /// orphan partials the index misses). Future usageBytes() async { final dir = Directory(await _tracksDir()); if (!await dir.exists()) return 0; var total = 0; - await for (final entity in dir.list(followLinks: false)) { - if (entity is File) { + await for (final e in dir.list(followLinks: false)) { + if (e is File) { try { - total += await entity.length(); - } catch (_) { - // Race against concurrent writes / deletes — just skip. - } + total += await e.length(); + } catch (_) {} } } return total; } - /// Evicts files until usage ≤ targetBytes. Eviction order: - /// incidental → autoPrefetch → autoPlaylist → autoLiked. - /// `manual` never evicts via this path; only clearAll() removes them. - Future evict({required int targetBytes}) async { - final used = await usageBytes(); - if (used <= targetBytes) return; - final order = [ - CacheSource.incidental, - CacheSource.autoPrefetch, - CacheSource.autoPlaylist, - CacheSource.autoLiked, - ]; - int remaining = used - targetBytes; - for (final src in order) { - if (remaining <= 0) break; - final rows = await (_db.select(_db.audioCacheIndex) - ..where((t) => t.source.equalsValue(src)) - ..orderBy([(t) => OrderingTerm.asc(t.cachedAt)])) + /// Bytes per bucket. Indexed rows split by liked-ness; on-disk + /// files with no index row (orphan partials) count as Rolling so + /// the rolling cap genuinely bounds disk. + Future bucketUsage(Set liked) async { + final rows = await _db.select(_db.audioCacheIndex).get(); + final indexed = {}; + var likedB = 0; + var rollingB = 0; + for (final r in rows) { + indexed.add(r.trackId); + if (liked.contains(r.trackId)) { + likedB += r.sizeBytes; + } else { + rollingB += r.sizeBytes; + } + } + final dir = Directory(await _tracksDir()); + if (await dir.exists()) { + await for (final e in dir.list(followLinks: false)) { + if (e is! File) continue; + final name = e.uri.pathSegments.last; + final id = name.endsWith('.mp3') + ? name.substring(0, name.length - 4) + : name; + if (indexed.contains(id)) continue; + try { + rollingB += await e.length(); + } catch (_) {} + } + } + return (liked: likedB, rolling: rollingB); + } + + /// Enforces both budgets independently (0 = unlimited). + /// + /// Rolling: evict non-liked indexed rows LRU (oldest lastPlayedAt + /// /cachedAt first), then sweep orphan files oldest-by-mtime, until + /// rolling usage ≤ rollingCap. Liked: evict liked indexed rows LRU + /// until ≤ likedCap. Liked is only ever touched by its own (large) + /// cap, so normal use never evicts the user's liked library. + Future evictBuckets({ + required int likedCap, + required int rollingCap, + required Set liked, + }) async { + final usage = await bucketUsage(liked); + + if (rollingCap > 0 && usage.rolling > rollingCap) { + var over = usage.rolling - rollingCap; + final rolling = await (_db.select(_db.audioCacheIndex) + ..where((t) => t.trackId.isNotIn(liked.toList())) + ..orderBy([ + (t) => OrderingTerm.asc(t.lastPlayedAt), + (t) => OrderingTerm.asc(t.cachedAt), + ])) .get(); - for (final row in rows) { - if (remaining <= 0) break; - final f = File(row.path); + for (final r in rolling) { + if (over <= 0) break; + final f = File(r.path); if (f.existsSync()) await f.delete(); await (_db.delete(_db.audioCacheIndex) - ..where((t) => t.trackId.equals(row.trackId))) + ..where((t) => t.trackId.equals(r.trackId))) .go(); - remaining -= row.sizeBytes; + over -= r.sizeBytes; + } + if (over > 0) await _sweepOrphans(over); + } + + if (likedCap > 0 && usage.liked > likedCap) { + var over = usage.liked - likedCap; + final likedRows = await (_db.select(_db.audioCacheIndex) + ..where((t) => t.trackId.isIn(liked.toList())) + ..orderBy([ + (t) => OrderingTerm.asc(t.lastPlayedAt), + (t) => OrderingTerm.asc(t.cachedAt), + ])) + .get(); + for (final r in likedRows) { + if (over <= 0) break; + final f = File(r.path); + if (f.existsSync()) await f.delete(); + await (_db.delete(_db.audioCacheIndex) + ..where((t) => t.trackId.equals(r.trackId))) + .go(); + over -= r.sizeBytes; } } } - /// Clears EVERY row + EVERY file. Wired to the operator's "Clear cache" - /// button — `manual` source rows are removed here but only here. + /// Deletes orphan files (on disk, no index row) oldest-mtime-first + /// until `over` bytes are reclaimed. These are unindexed partials, + /// always Rolling, always evict-first. + Future _sweepOrphans(int over) async { + final indexed = { + for (final r in await _db.select(_db.audioCacheIndex).get()) r.trackId + }; + final dir = Directory(await _tracksDir()); + if (!await dir.exists()) return; + final orphans = <({File f, int size, DateTime mtime})>[]; + await for (final e in dir.list(followLinks: false)) { + if (e is! File) continue; + final name = e.uri.pathSegments.last; + final id = + name.endsWith('.mp3') ? name.substring(0, name.length - 4) : name; + if (indexed.contains(id)) continue; + try { + final st = e.statSync(); + orphans.add((f: e, size: st.size, mtime: st.modified)); + } catch (_) {} + } + orphans.sort((a, b) => a.mtime.compareTo(b.mtime)); + var remaining = over; + for (final o in orphans) { + if (remaining <= 0) break; + try { + await o.f.delete(); + remaining -= o.size; + } catch (_) {} + } + } + + /// Clears EVERY row + EVERY file. Wired to "Clear cache". Future clearAll() async { final dir = await _tracksDir(); final d = Directory(dir); if (d.existsSync()) { - for (final entity in d.listSync()) { - if (entity is File) await entity.delete(); + for (final e in d.listSync()) { + if (e is File) await e.delete(); } } await _db.delete(_db.audioCacheIndex).go(); diff --git a/flutter_client/lib/cache/cache_settings_provider.dart b/flutter_client/lib/cache/cache_settings_provider.dart index 6a17e609..517ab195 100644 --- a/flutter_client/lib/cache/cache_settings_provider.dart +++ b/flutter_client/lib/cache/cache_settings_provider.dart @@ -3,17 +3,29 @@ import 'package:flutter_secure_storage/flutter_secure_storage.dart'; import '../auth/auth_provider.dart' show secureStorageProvider; -/// Operator-tunable cache settings (#357 plan B). Persisted via +/// Operator-tunable cache settings. Persisted via /// flutter_secure_storage on the same device. +/// +/// #427 S2: the single `capBytes` is replaced by two independent +/// budgets — Liked and Rolling-recent — each defaulting to 5GB. +/// Bucketing is by liked-ness (a cached track currently in the +/// user's liked set is charged to Liked; everything else to +/// Rolling), so the dedup is storage-only and never filters +/// playback. The old `cache_cap_bytes` key is intentionally not +/// migrated; defaults reapply (a one-time re-tune, not data loss). class CacheSettings { const CacheSettings({ - required this.capBytes, + required this.likedCapBytes, + required this.rollingCapBytes, required this.prefetchWindow, required this.cacheLikedTracks, }); - /// 0 = unlimited. - final int capBytes; + /// Liked-bucket budget. 0 = unlimited. + final int likedCapBytes; + + /// Rolling (recently-played) budget. 0 = unlimited. + final int rollingCapBytes; /// 1..10. Number of next-tracks the prefetcher pre-downloads. final int prefetchWindow; @@ -22,25 +34,31 @@ class CacheSettings { final bool cacheLikedTracks; CacheSettings copyWith({ - int? capBytes, + int? likedCapBytes, + int? rollingCapBytes, int? prefetchWindow, bool? cacheLikedTracks, }) => CacheSettings( - capBytes: capBytes ?? this.capBytes, + likedCapBytes: likedCapBytes ?? this.likedCapBytes, + rollingCapBytes: rollingCapBytes ?? this.rollingCapBytes, prefetchWindow: prefetchWindow ?? this.prefetchWindow, cacheLikedTracks: cacheLikedTracks ?? this.cacheLikedTracks, ); + static const _fiveGiB = 5 * 1024 * 1024 * 1024; + static const defaults = CacheSettings( - capBytes: 5 * 1024 * 1024 * 1024, + likedCapBytes: _fiveGiB, + rollingCapBytes: _fiveGiB, prefetchWindow: 5, cacheLikedTracks: true, ); } class CacheSettingsController extends AsyncNotifier { - static const _kCap = 'cache_cap_bytes'; + static const _kLikedCap = 'cache_liked_cap_bytes'; + static const _kRollingCap = 'cache_rolling_cap_bytes'; static const _kPrefetch = 'cache_prefetch_window'; static const _kCacheLiked = 'cache_liked_tracks'; @@ -49,13 +67,17 @@ class CacheSettingsController extends AsyncNotifier { @override Future build() async { _storage = ref.read(secureStorageProvider); - final cap = await _storage.read(key: _kCap); + final likedCap = await _storage.read(key: _kLikedCap); + final rollingCap = await _storage.read(key: _kRollingCap); final pre = await _storage.read(key: _kPrefetch); final liked = await _storage.read(key: _kCacheLiked); return CacheSettings( - capBytes: cap == null - ? CacheSettings.defaults.capBytes - : int.tryParse(cap) ?? CacheSettings.defaults.capBytes, + likedCapBytes: likedCap == null + ? CacheSettings.defaults.likedCapBytes + : int.tryParse(likedCap) ?? CacheSettings.defaults.likedCapBytes, + rollingCapBytes: rollingCap == null + ? CacheSettings.defaults.rollingCapBytes + : int.tryParse(rollingCap) ?? CacheSettings.defaults.rollingCapBytes, prefetchWindow: pre == null ? CacheSettings.defaults.prefetchWindow : (int.tryParse(pre) ?? 5).clamp(1, 10), @@ -65,9 +87,14 @@ class CacheSettingsController extends AsyncNotifier { ); } - Future setCapBytes(int bytes) async { - await _storage.write(key: _kCap, value: bytes.toString()); - state = AsyncData(state.value!.copyWith(capBytes: bytes)); + Future setLikedCapBytes(int bytes) async { + await _storage.write(key: _kLikedCap, value: bytes.toString()); + state = AsyncData(state.value!.copyWith(likedCapBytes: bytes)); + } + + Future setRollingCapBytes(int bytes) async { + await _storage.write(key: _kRollingCap, value: bytes.toString()); + state = AsyncData(state.value!.copyWith(rollingCapBytes: bytes)); } Future setPrefetchWindow(int n) async { diff --git a/flutter_client/lib/cache/db.dart b/flutter_client/lib/cache/db.dart index 26b79754..824adcb9 100644 --- a/flutter_client/lib/cache/db.dart +++ b/flutter_client/lib/cache/db.dart @@ -101,6 +101,11 @@ class AudioCacheIndex extends Table { TextColumn get path => text()(); IntColumn get sizeBytes => integer()(); DateTimeColumn get cachedAt => dateTime().withDefault(currentDateAndTime)(); + /// When the track was last played. Drives the offline "Recently + /// played" ordering and rolling-bucket LRU eviction. Distinct from + /// cachedAt (download time). Nullable for pre-schema-9 rows until + /// the next play touches them (migration backfills to cachedAt). + DateTimeColumn get lastPlayedAt => dateTime().nullable()(); TextColumn get source => textEnum()(); @override Set get primaryKey => {trackId}; @@ -247,7 +252,7 @@ class AppDb extends _$AppDb { AppDb([QueryExecutor? e]) : super(e ?? driftDatabase(name: 'minstrel_cache')); @override - int get schemaVersion => 8; + int get schemaVersion => 9; @override MigrationStrategy get migration => MigrationStrategy( @@ -300,6 +305,17 @@ class AppDb extends _$AppDb { // upgrade; populated by controllers when REST calls fail. await m.createTable(cachedMutations); } + if (from < 9) { + // Schema 9 (#427 S2): two-bucket cache. lastPlayedAt + // gives the offline "Recently played" view a real + // recency signal (cachedAt is download time, not play + // time). Nullable — backfilled to cachedAt so existing + // rows order sensibly until next play touches them. + await m.addColumn(audioCacheIndex, audioCacheIndex.lastPlayedAt); + await customStatement( + 'UPDATE audio_cache_index SET last_played_at = cached_at', + ); + } }, ); } diff --git a/flutter_client/lib/cache/prefetcher.dart b/flutter_client/lib/cache/prefetcher.dart index 785d05f6..2abeaf8e 100644 --- a/flutter_client/lib/cache/prefetcher.dart +++ b/flutter_client/lib/cache/prefetcher.dart @@ -3,6 +3,7 @@ import 'dart:async'; import 'package:audio_service/audio_service.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; +import '../likes/likes_provider.dart' show likedIdsProvider; import '../player/album_color_extractor.dart'; import '../player/player_provider.dart'; import 'audio_cache_manager.dart'; @@ -86,10 +87,17 @@ class Prefetcher { } } - // Eviction pass after pinning new files. - if (settings.capBytes > 0) { - await mgr.evict(targetBytes: settings.capBytes); - } + // Eviction pass after pinning new files. Per-bucket (#427 S2): + // liked-ness decides the bucket, so a track that's both liked + // and recently played is protected by the Liked cap, not the + // rolling LRU. + final liked = + _ref.read(likedIdsProvider).value?.tracks ?? const {}; + await mgr.evictBuckets( + likedCap: settings.likedCapBytes, + rollingCap: settings.rollingCapBytes, + liked: liked, + ); } } diff --git a/flutter_client/lib/library/album_detail_screen.dart b/flutter_client/lib/library/album_detail_screen.dart index 8e11844e..9b628906 100644 --- a/flutter_client/lib/library/album_detail_screen.dart +++ b/flutter_client/lib/library/album_detail_screen.dart @@ -3,8 +3,6 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../api/endpoints/likes.dart'; import '../models/album.dart'; -import '../cache/audio_cache_manager.dart'; -import '../cache/db.dart'; import '../likes/like_button.dart'; import '../player/player_provider.dart'; import '../shared/widgets/server_image.dart'; @@ -113,21 +111,6 @@ class AlbumDetailScreen extends ConsumerWidget { Text(r.album.artistName, style: TextStyle(color: fs.ash)), ], )), - IconButton( - key: const Key('download_album_button'), - icon: Icon(Icons.download, color: fs.ash), - tooltip: 'Download album', - onPressed: () { - final mgr = ref.read(audioCacheManagerProvider); - for (final t in r.tracks) { - // ignore: unawaited_futures - mgr.pin(t.id, source: CacheSource.autoPlaylist); - } - ScaffoldMessenger.of(context).showSnackBar( - SnackBar(content: Text('Downloading ${r.tracks.length} tracks…')), - ); - }, - ), Container( width: 48, height: 48, decoration: BoxDecoration(color: fs.accent, shape: BoxShape.circle), diff --git a/flutter_client/lib/playlists/playlist_detail_screen.dart b/flutter_client/lib/playlists/playlist_detail_screen.dart index b53cc7d2..ddb19d77 100644 --- a/flutter_client/lib/playlists/playlist_detail_screen.dart +++ b/flutter_client/lib/playlists/playlist_detail_screen.dart @@ -2,8 +2,6 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:go_router/go_router.dart'; -import '../cache/audio_cache_manager.dart'; -import '../cache/db.dart'; import '../models/playlist.dart'; import '../models/track.dart'; import '../player/player_provider.dart'; @@ -266,33 +264,15 @@ class _Header extends ConsumerWidget { ), const Spacer(), if (playable.isNotEmpty) ...[ - if (p.refreshable) + if (p.refreshable) ...[ OutlinedButton.icon( key: const Key('regenerate_playlist_button'), onPressed: () => _regenerate(context, ref), icon: const Icon(Icons.refresh, size: 16), label: const Text('Regenerate'), - ) - else - OutlinedButton.icon( - key: const Key('download_playlist_button'), - onPressed: () { - final mgr = ref.read(audioCacheManagerProvider); - for (final t in playable) { - final id = t.trackId ?? ''; - if (id.isEmpty) continue; - // Fire-and-forget; downloads happen in background. - // ignore: unawaited_futures - mgr.pin(id, source: CacheSource.autoPlaylist); - } - ScaffoldMessenger.of(context).showSnackBar( - SnackBar(content: Text('Downloading ${playable.length} tracks…')), - ); - }, - icon: const Icon(Icons.download, size: 16), - label: const Text('Download'), ), - const SizedBox(width: 8), + const SizedBox(width: 8), + ], FilledButton.icon( onPressed: () { final refs = playable.map(_toTrackRef).toList(growable: false); diff --git a/flutter_client/lib/settings/storage_section.dart b/flutter_client/lib/settings/storage_section.dart index f5488195..68e6b84c 100644 --- a/flutter_client/lib/settings/storage_section.dart +++ b/flutter_client/lib/settings/storage_section.dart @@ -4,10 +4,12 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../cache/audio_cache_manager.dart'; import '../cache/cache_settings_provider.dart'; import '../cache/sync_controller.dart'; +import '../likes/likes_provider.dart' show likedIdsProvider; import '../theme/theme_extension.dart'; -/// Settings card: usage display, cap selector, prefetch window selector, -/// cache-liked toggle, Clear cache + Sync now buttons. +/// Settings card: per-bucket usage, two cap selectors (Liked + +/// Recently-played), prefetch window, cache-liked toggle, Clear +/// cache + Sync now. #427 S2/S3: two independent budgets. class StorageSection extends ConsumerStatefulWidget { const StorageSection({super.key}); @@ -16,7 +18,7 @@ class StorageSection extends ConsumerStatefulWidget { } class _StorageSectionState extends ConsumerState { - int? _usageBytes; + BucketUsage? _usage; bool _syncing = false; @override @@ -25,10 +27,13 @@ class _StorageSectionState extends ConsumerState { _refreshUsage(); } + Set _likedSet() => + ref.read(likedIdsProvider).value?.tracks ?? const {}; + Future _refreshUsage() async { final mgr = ref.read(audioCacheManagerProvider); - final used = await mgr.usageBytes(); - if (mounted) setState(() => _usageBytes = used); + final u = await mgr.bucketUsage(_likedSet()); + if (mounted) setState(() => _usage = u); } String _fmtBytes(int? n) { @@ -36,10 +41,14 @@ class _StorageSectionState extends ConsumerState { if (n == 0) return '0 B'; if (n < 1024) return '$n B'; if (n < 1024 * 1024) return '${(n / 1024).toStringAsFixed(1)} KB'; - if (n < 1024 * 1024 * 1024) return '${(n / 1024 / 1024).toStringAsFixed(1)} MB'; + if (n < 1024 * 1024 * 1024) { + return '${(n / 1024 / 1024).toStringAsFixed(1)} MB'; + } return '${(n / 1024 / 1024 / 1024).toStringAsFixed(2)} GB'; } + String _cap(int bytes) => bytes == 0 ? 'unlimited' : _fmtBytes(bytes); + @override Widget build(BuildContext context) { final fs = Theme.of(context).extension()!; @@ -65,17 +74,32 @@ class _StorageSectionState extends ConsumerState { fontSize: 18, fontWeight: FontWeight.w500)), const SizedBox(height: 12), - Row(children: [ - Text('Cache usage', style: TextStyle(color: fs.ash)), - const Spacer(), - Text( - '${_fmtBytes(_usageBytes)} / ' - '${s.capBytes == 0 ? "unlimited" : _fmtBytes(s.capBytes)}', - style: TextStyle(color: fs.parchment), - ), - ]), + _usageRow('Liked', _usage?.liked, s.likedCapBytes, fs), + const SizedBox(height: 4), + _usageRow('Recently played', _usage?.rolling, + s.rollingCapBytes, fs), const SizedBox(height: 16), - _capSelector(s, fs), + _capSelector( + 'Liked cache limit', + const Key('liked_cap_selector'), + s.likedCapBytes, + (v) => ref + .read(cacheSettingsProvider.notifier) + .setLikedCapBytes(v), + s, + fs, + ), + const SizedBox(height: 8), + _capSelector( + 'Recently-played cache limit', + const Key('rolling_cap_selector'), + s.rollingCapBytes, + (v) => ref + .read(cacheSettingsProvider.notifier) + .setRollingCapBytes(v), + s, + fs, + ), const SizedBox(height: 8), _prefetchSelector(s, fs), const SizedBox(height: 8), @@ -128,29 +152,52 @@ class _StorageSectionState extends ConsumerState { ); } - Widget _capSelector(CacheSettings s, FabledSwordTheme fs) { - const options = [ - (1024 * 1024 * 1024, '1 GB'), - (5 * 1024 * 1024 * 1024, '5 GB'), - (10 * 1024 * 1024 * 1024, '10 GB'), - (25 * 1024 * 1024 * 1024, '25 GB'), - (0, 'Unlimited'), - ]; + Widget _usageRow(String label, int? used, int cap, FabledSwordTheme fs) { return Row(children: [ - Expanded(child: Text('Cache size limit', style: TextStyle(color: fs.ash))), + Text(label, style: TextStyle(color: fs.ash)), + const Spacer(), + Text('${_fmtBytes(used)} / ${_cap(cap)}', + style: TextStyle(color: fs.parchment)), + ]); + } + + static const _capOptions = [ + (1024 * 1024 * 1024, '1 GB'), + (5 * 1024 * 1024 * 1024, '5 GB'), + (10 * 1024 * 1024 * 1024, '10 GB'), + (25 * 1024 * 1024 * 1024, '25 GB'), + (0, 'Unlimited'), + ]; + + Widget _capSelector( + String label, + Key key, + int current, + Future Function(int) setCap, + CacheSettings s, + FabledSwordTheme fs, + ) { + return Row(children: [ + Expanded(child: Text(label, style: TextStyle(color: fs.ash))), DropdownButton( - key: const Key('cap_selector'), - value: options.any((o) => o.$1 == s.capBytes) - ? s.capBytes + key: key, + value: _capOptions.any((o) => o.$1 == current) + ? current : 5 * 1024 * 1024 * 1024, - items: options + items: _capOptions .map((o) => DropdownMenuItem(value: o.$1, child: Text(o.$2))) .toList(), onChanged: (v) async { if (v == null) return; - await ref.read(cacheSettingsProvider.notifier).setCapBytes(v); - if (v > 0) { - await ref.read(audioCacheManagerProvider).evict(targetBytes: v); + await setCap(v); + // Enforce immediately against the freshest settings. + final fresh = ref.read(cacheSettingsProvider).value; + if (fresh != null) { + await ref.read(audioCacheManagerProvider).evictBuckets( + likedCap: fresh.likedCapBytes, + rollingCap: fresh.rollingCapBytes, + liked: _likedSet(), + ); } await _refreshUsage(); }, @@ -161,7 +208,8 @@ class _StorageSectionState extends ConsumerState { Widget _prefetchSelector(CacheSettings s, FabledSwordTheme fs) { const options = [1, 3, 5, 7, 10]; return Row(children: [ - Expanded(child: Text('Pre-fetch ahead', style: TextStyle(color: fs.ash))), + Expanded( + child: Text('Pre-fetch ahead', style: TextStyle(color: fs.ash))), DropdownButton( key: const Key('prefetch_selector'), value: options.contains(s.prefetchWindow) ? s.prefetchWindow : 5, @@ -183,8 +231,8 @@ class _StorageSectionState extends ConsumerState { builder: (ctx) => AlertDialog( title: const Text('Clear cache?'), content: const Text( - 'This deletes all cached audio files (including manually downloaded ones). ' - 'The next play of any track will re-download from the server.', + 'This deletes all cached audio (liked and recently-played). ' + 'The next play of any track re-downloads from the server.', ), actions: [ TextButton( diff --git a/flutter_client/test/cache/audio_cache_manager_test.dart b/flutter_client/test/cache/audio_cache_manager_test.dart index 669279f1..433f5ae1 100644 --- a/flutter_client/test/cache/audio_cache_manager_test.dart +++ b/flutter_client/test/cache/audio_cache_manager_test.dart @@ -4,6 +4,7 @@ library; import 'dart:io'; import 'package:dio/dio.dart'; +import 'package:drift/drift.dart' show Value; import 'package:drift/native.dart' show NativeDatabase; import 'package:flutter_test/flutter_test.dart'; @@ -54,7 +55,8 @@ void main() { expect(await mgr.usageBytes(), 350); }); - test('eviction order: incidental first, manual never', skip: _skipDrift, () async { + test('rolling cap evicts non-liked LRU; liked protected', + skip: _skipDrift, () async { final db = _testDb(); addTearDown(db.close); final tmp = Directory.systemTemp.createTempSync(); @@ -63,30 +65,47 @@ void main() { dioFactory: () async => Dio(), cacheDirFactory: () async => tmp, ); - final f1 = File('${tmp.path}/audio_cache/inc.mp3'); - await f1.create(recursive: true); - await f1.writeAsBytes(List.filled(100, 0)); - final f2 = File('${tmp.path}/audio_cache/man.mp3'); - await f2.create(recursive: true); - await f2.writeAsBytes(List.filled(100, 0)); + Future mk(String id) async { + final f = File('${tmp.path}/audio_cache/$id.mp3'); + await f.create(recursive: true); + await f.writeAsBytes(List.filled(100, 0)); + } + await mk('old'); + await mk('new'); + await mk('lik'); await db.batch((b) { b.insertAll(db.audioCacheIndex, [ + // 'old' has the older lastPlayedAt → evicted first. AudioCacheIndexCompanion.insert( - trackId: 'inc', - path: f1.path, + trackId: 'old', + path: '${tmp.path}/audio_cache/old.mp3', sizeBytes: 100, - source: CacheSource.incidental), + source: CacheSource.incidental, + lastPlayedAt: Value(DateTime(2020))), AudioCacheIndexCompanion.insert( - trackId: 'man', - path: f2.path, + trackId: 'new', + path: '${tmp.path}/audio_cache/new.mp3', sizeBytes: 100, - source: CacheSource.manual), + source: CacheSource.incidental, + lastPlayedAt: Value(DateTime(2024))), + AudioCacheIndexCompanion.insert( + trackId: 'lik', + path: '${tmp.path}/audio_cache/lik.mp3', + sizeBytes: 100, + source: CacheSource.incidental, + lastPlayedAt: Value(DateTime(2019))), ]); }); - expect(await mgr.usageBytes(), 200); - await mgr.evict(targetBytes: 100); - expect(await mgr.isCached('inc'), false); - expect(await mgr.isCached('man'), true); + final liked = {'lik'}; + final u = await mgr.bucketUsage(liked); + expect(u.liked, 100); + expect(u.rolling, 200); + // Rolling cap fits one 100-byte file; Liked cap huge. + await mgr.evictBuckets( + likedCap: 1 << 30, rollingCap: 100, liked: liked); + expect(await mgr.isCached('old'), false); // oldest rolling, evicted + expect(await mgr.isCached('new'), true); // newer rolling, kept + expect(await mgr.isCached('lik'), true); // liked, protected }); test('clearAll removes everything including manual', skip: _skipDrift, () async { diff --git a/flutter_client/test/cache/cache_settings_provider_test.dart b/flutter_client/test/cache/cache_settings_provider_test.dart index 0df3681d..284800e4 100644 --- a/flutter_client/test/cache/cache_settings_provider_test.dart +++ b/flutter_client/test/cache/cache_settings_provider_test.dart @@ -27,7 +27,9 @@ void main() { addTearDown(container.dispose); final s = await container.read(cacheSettingsProvider.future); - expect(s.capBytes, CacheSettings.defaults.capBytes); + expect(s.likedCapBytes, CacheSettings.defaults.likedCapBytes); + expect(s.rollingCapBytes, CacheSettings.defaults.rollingCapBytes); + expect(s.likedCapBytes, 5 * 1024 * 1024 * 1024); expect(s.prefetchWindow, 5); expect(s.cacheLikedTracks, true); }); diff --git a/flutter_client/test/settings/storage_section_test.dart b/flutter_client/test/settings/storage_section_test.dart index 816d2815..f4711bb9 100644 --- a/flutter_client/test/settings/storage_section_test.dart +++ b/flutter_client/test/settings/storage_section_test.dart @@ -46,12 +46,14 @@ void main() { await t.pumpAndSettle(); expect(find.text('Storage'), findsOneWidget); - expect(find.text('Cache size limit'), findsOneWidget); + expect(find.text('Liked cache limit'), findsOneWidget); + expect(find.text('Recently-played cache limit'), findsOneWidget); expect(find.text('Pre-fetch ahead'), findsOneWidget); expect(find.byKey(const Key('clear_cache_button')), findsOneWidget); expect(find.byKey(const Key('sync_now_button')), findsOneWidget); expect(find.byKey(const Key('cache_liked_toggle')), findsOneWidget); - expect(find.byKey(const Key('cap_selector')), findsOneWidget); + expect(find.byKey(const Key('liked_cap_selector')), findsOneWidget); + expect(find.byKey(const Key('rolling_cap_selector')), findsOneWidget); expect(find.byKey(const Key('prefetch_selector')), findsOneWidget); }); From a4f293b7cfcfebc942516bef1778cb5f7de784fd Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 21:20:10 -0400 Subject: [PATCH 21/27] ci(flutter): build debug APK on main only, not every dev push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The debug APK step dominated the run (~7m32s of ~10m) and ran on every dev push, but the operator dev-tests via a local Android Studio build, not the CI artifact. Gate the debug APK + its artifact upload to main pushes only: dev = analyze+test+codegen (~3min); main = + debug APK as the native-build safety net (Gradle/manifest/plugin breakage analyze+test can't catch) before any release tag; tags = signed release APK (unchanged). Note: the bulk of that 7m32s was the flutter-ci runner image re-installing NDK 28.2.13676358 + build-tools 35 + platform 35/36 + cmake 3.22.1 every run (image baked platform-34/build-tools-34, stale vs the bundled Flutter's requirements). Baking those into CI-Runner/CI-flutter/Dockerfile is the larger win and also speeds release builds — tracked separately (operator-side infra, not in this repo). Co-Authored-By: Claude Opus 4.7 (1M context) --- .forgejo/workflows/flutter.yml | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/.forgejo/workflows/flutter.yml b/.forgejo/workflows/flutter.yml index b677f62b..5f375e69 100644 --- a/.forgejo/workflows/flutter.yml +++ b/.forgejo/workflows/flutter.yml @@ -1,9 +1,13 @@ name: flutter -# Analyze + test + build the Flutter mobile client. Only runs when the -# diff touches flutter_client/ or one of the shared web inputs the -# sync_shared.sh script copies. Other pushes skip — this workflow is -# independent from the Go/web test.yml and release.yml. +# Analyze + test the Flutter mobile client; build an APK only where +# it's actually consumed. dev pushes: analyze+test+codegen only +# (~3min) — the operator dev-tests via a local Android Studio build, +# not the CI artifact. main pushes: also build a debug APK as the +# native-build safety net (Gradle/manifest/plugin breakage that +# analyze+test can't catch) before any release tag. tags: signed +# release APK. Only runs when the diff touches flutter_client/ or a +# shared web input sync_shared.sh copies. on: push: @@ -69,7 +73,9 @@ jobs: run: flutter test --reporter compact - name: Build debug APK - if: github.event_name == 'push' && !startsWith(github.ref, 'refs/tags/') + # main-only: native-build safety net before release tags. + # dev skips this (~7min) — operator dev-tests locally. + if: github.event_name == 'push' && github.ref == 'refs/heads/main' run: flutter build apk --debug - name: Decode signing keystore @@ -112,7 +118,7 @@ jobs: flutter build apk --release --build-name="${TAG}" - name: Upload debug APK artifact - if: github.event_name == 'push' && !startsWith(github.ref, 'refs/tags/') + if: github.event_name == 'push' && github.ref == 'refs/heads/main' uses: forgejo/upload-artifact@v3 with: name: minstrel-debug-${{ github.sha }} From a5e2abb8c42ffad9c77a7b81f5f274bd8a9c5adc Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 21:37:29 -0400 Subject: [PATCH 22/27] =?UTF-8?q?feat(offline):=20#427=20S4a=20=E2=80=94?= =?UTF-8?q?=20Shuffle=20all=20+=20offline=20system-playlist=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Headline of S4. "Shuffle all" is always present (home app-bar shuffle icon); the pool degrades with reachability: - online → GET /api/library/shuffle?limit=N (new): N random library tracks server-side, per-user quarantine filtered (ListRandomTracksForUser, ORDER BY random()). - offline → ShuffleSource shuffles the whole local cache index (audio_cache_index ∩ cached_tracks, names from cached_artists/ albums) — a UNION over liked AND recently-played, since the two-bucket split is storage-only and never filters playback. Offline gating: refreshable (singleton) system playlists need the live build/shuffle endpoints, so their tile play is disabled when offlineProvider is true — Shuffle all is the offline path. User playlists still play from cache. playlist_card now reads offlineProvider in build → wrapped the direct-render widget test in ProviderScope (offlineProvider is smoke-safe from S1: tracked timers, no connectivity mount). S4b (offline Recently-played / Liked browsable surfaces over the cache index) next. Co-Authored-By: Claude Opus 4.7 (1M context) --- flutter_client/lib/api/endpoints/library.dart | 13 ++++ flutter_client/lib/cache/shuffle_source.dart | 68 +++++++++++++++++++ flutter_client/lib/library/home_screen.dart | 25 ++++++- .../lib/playlists/widgets/playlist_card.dart | 8 ++- .../playlists/widgets/playlist_card_test.dart | 21 +++--- internal/api/api.go | 1 + internal/api/library.go | 38 +++++++++++ internal/db/dbq/tracks.sql.go | 67 ++++++++++++++++++ internal/db/queries/tracks.sql | 17 +++++ 9 files changed, 248 insertions(+), 10 deletions(-) create mode 100644 flutter_client/lib/cache/shuffle_source.dart diff --git a/flutter_client/lib/api/endpoints/library.dart b/flutter_client/lib/api/endpoints/library.dart index e5cfff71..53895b7f 100644 --- a/flutter_client/lib/api/endpoints/library.dart +++ b/flutter_client/lib/api/endpoints/library.dart @@ -85,6 +85,19 @@ class LibraryApi { .toList(growable: false); } + /// GET /api/library/shuffle?limit=N (#427 S4). N random library + /// tracks — the online source for "Shuffle all". Bare JSON array. + Future> shuffle({int limit = 100}) async { + final r = await _dio.get>( + '/api/library/shuffle', + queryParameters: {'limit': limit}, + ); + final raw = r.data ?? const []; + return raw + .map((e) => TrackRef.fromJson((e as Map).cast())) + .toList(growable: false); + } + /// GET /api/artists/{id}/tracks. Server emits a bare JSON array, so we /// type the response as `List` rather than a Map envelope. Future> getArtistTracks(String id) async { diff --git a/flutter_client/lib/cache/shuffle_source.dart b/flutter_client/lib/cache/shuffle_source.dart new file mode 100644 index 00000000..5188a5fd --- /dev/null +++ b/flutter_client/lib/cache/shuffle_source.dart @@ -0,0 +1,68 @@ +// "Shuffle all" source (#427 S4). Always-present; the pool degrades +// gracefully with reachability: +// online → GET /api/library/shuffle (random over the whole library) +// offline → a client shuffle over the entire local cache index +// +// Offline is a UNION over every cached track regardless of storage +// bucket — liked AND recently-played both included. The two-bucket +// split (S2) is storage/eviction-only and never filters playback, +// which is exactly the property this relies on. + +import 'dart:math'; + +import 'package:drift/drift.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; + +import '../library/library_providers.dart' show libraryApiProvider; +import '../models/track.dart'; +import 'adapters.dart'; +import 'audio_cache_manager.dart' show appDbProvider; +import 'offline_provider.dart'; + +class ShuffleSource { + ShuffleSource(this._ref); + final Ref _ref; + + /// Returns up to [limit] tracks to play as a shuffle-all. Online: + /// server-random. Offline: every cached track (liked + recent), + /// shuffled client-side. Empty offline → nothing is cached yet. + Future> tracks({int limit = 100}) async { + if (_ref.read(offlineProvider)) { + return _offline(limit); + } + final api = await _ref.read(libraryApiProvider.future); + return api.shuffle(limit: limit); + } + + Future> _offline(int limit) async { + final db = _ref.read(appDbProvider); + final cachedIds = + (await db.select(db.audioCacheIndex).get()).map((r) => r.trackId).toSet(); + if (cachedIds.isEmpty) return const []; + + final meta = await (db.select(db.cachedTracks) + ..where((t) => t.id.isIn(cachedIds.toList()))) + .get(); + if (meta.isEmpty) return const []; + + final artistName = { + for (final a in await db.select(db.cachedArtists).get()) a.id: a.name + }; + final albumTitle = { + for (final a in await db.select(db.cachedAlbums).get()) a.id: a.title + }; + + final refs = [ + for (final t in meta) + t.toRef( + artistName: artistName[t.artistId] ?? '', + albumTitle: albumTitle[t.albumId] ?? '', + ) + ]; + refs.shuffle(Random()); + return refs.length > limit ? refs.sublist(0, limit) : refs; + } +} + +final shuffleSourceProvider = + Provider((ref) => ShuffleSource(ref)); diff --git a/flutter_client/lib/library/home_screen.dart b/flutter_client/lib/library/home_screen.dart index 968ed772..c736a31f 100644 --- a/flutter_client/lib/library/home_screen.dart +++ b/flutter_client/lib/library/home_screen.dart @@ -4,7 +4,9 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:go_router/go_router.dart'; import '../api/errors.dart'; +import '../cache/shuffle_source.dart'; import '../cache/tile_providers.dart'; +import '../player/player_provider.dart'; import '../models/playlist.dart'; import '../models/system_playlists_status.dart'; import '../models/track.dart'; @@ -41,7 +43,28 @@ class HomeScreen extends ConsumerWidget { backgroundColor: fs.obsidian, elevation: 0, title: Text('Minstrel', style: TextStyle(color: fs.parchment)), - actions: const [MainAppBarActions(currentRoute: '/home')], + actions: [ + IconButton( + key: const Key('shuffle_all_button'), + tooltip: 'Shuffle all', + icon: Icon(Icons.shuffle, color: fs.parchment), + onPressed: () async { + final messenger = ScaffoldMessenger.of(context); + final refs = + await ref.read(shuffleSourceProvider).tracks(); + if (refs.isEmpty) { + messenger.showSnackBar(const SnackBar( + content: Text('Nothing to shuffle yet'), + )); + return; + } + await ref + .read(playerActionsProvider) + .playTracks(refs, shuffle: true); + }, + ), + const MainAppBarActions(currentRoute: '/home'), + ], ), body: SafeArea( child: index.when( diff --git a/flutter_client/lib/playlists/widgets/playlist_card.dart b/flutter_client/lib/playlists/widgets/playlist_card.dart index 16cfaef0..cf439056 100644 --- a/flutter_client/lib/playlists/widgets/playlist_card.dart +++ b/flutter_client/lib/playlists/widgets/playlist_card.dart @@ -2,6 +2,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:go_router/go_router.dart'; +import '../../cache/offline_provider.dart'; import '../../library/widgets/play_circle_button.dart'; import '../../models/playlist.dart'; import '../../models/track.dart'; @@ -23,7 +24,12 @@ class PlaylistCard extends ConsumerWidget { @override Widget build(BuildContext context, WidgetRef ref) { final fs = Theme.of(context).extension()!; - final hasTracks = playlist.trackCount > 0; + // #427 S4: refreshable system playlists need the live build/ + // shuffle endpoints — unavailable offline. Disable their play + // (use Shuffle all instead). User playlists play from cache. + final offline = ref.watch(offlineProvider); + final hasTracks = playlist.trackCount > 0 && + !(offline && playlist.refreshable); return SizedBox( width: 176, child: Material( diff --git a/flutter_client/test/playlists/widgets/playlist_card_test.dart b/flutter_client/test/playlists/widgets/playlist_card_test.dart index 228a9707..69086907 100644 --- a/flutter_client/test/playlists/widgets/playlist_card_test.dart +++ b/flutter_client/test/playlists/widgets/playlist_card_test.dart @@ -1,4 +1,5 @@ import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:minstrel/models/playlist.dart'; @@ -35,44 +36,48 @@ const _forYou = Playlist( void main() { testWidgets('renders name and no badge for user playlist', (tester) async { - await tester.pumpWidget(MaterialApp( + await tester.pumpWidget(ProviderScope( + child: MaterialApp( theme: buildThemeData(), home: const Scaffold( body: PlaylistCard(playlist: _userPlaylist), ), - )); + ))); expect(find.text('Road trip'), findsOneWidget); expect(find.text('for you'), findsNothing); }); testWidgets('renders system badge for system playlist', (tester) async { - await tester.pumpWidget(MaterialApp( + await tester.pumpWidget(ProviderScope( + child: MaterialApp( theme: buildThemeData(), home: const Scaffold( body: PlaylistCard(playlist: _forYou), ), - )); + ))); expect(find.text('For You'), findsOneWidget); expect(find.text('for you'), findsOneWidget); }); testWidgets('shows refresh kebab on system playlists', (tester) async { - await tester.pumpWidget(MaterialApp( + await tester.pumpWidget(ProviderScope( + child: MaterialApp( theme: buildThemeData(), home: const Scaffold( body: PlaylistCard(playlist: _forYou), ), - )); + ))); expect(find.byIcon(Icons.more_vert), findsOneWidget); }); testWidgets('no refresh kebab on user playlists', (tester) async { - await tester.pumpWidget(MaterialApp( + await tester.pumpWidget(ProviderScope( + child: MaterialApp( theme: buildThemeData(), home: const Scaffold( body: PlaylistCard(playlist: _userPlaylist), ), - )); + ))); expect(find.byIcon(Icons.more_vert), findsNothing); }); } diff --git a/internal/api/api.go b/internal/api/api.go index 52878972..4f724493 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -74,6 +74,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Get("/artists/{id}/tracks", h.handleGetArtistTracks) authed.Get("/albums/{id}", h.handleGetAlbum) authed.Get("/albums/{id}/cover", h.handleGetCover) + authed.Get("/library/shuffle", h.handleLibraryShuffle) authed.Get("/library/albums", h.handleListLibraryAlbums) authed.Get("/library/sync", h.handleLibrarySync) authed.Get("/tracks/{id}", h.handleGetTrack) diff --git a/internal/api/library.go b/internal/api/library.go index 206c5549..1bfc7523 100644 --- a/internal/api/library.go +++ b/internal/api/library.go @@ -3,6 +3,7 @@ package api import ( "errors" "net/http" + "strconv" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" @@ -158,6 +159,43 @@ func (h *handlers) handleGetArtistTracks(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, out) } +// handleLibraryShuffle implements GET /api/library/shuffle?limit=N — +// the online source for the client's always-present "Shuffle all" +// (#427 S4). N random tracks across the whole library, per-user +// quarantine filtered. limit defaults to 100, clamped to 1..500. +// Offline, the client shuffles its local cache instead and never +// calls this. +func (h *handlers) handleLibraryShuffle(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + limit := 100 + if v := r.URL.Query().Get("limit"); v != "" { + if n, err := strconv.Atoi(v); err == nil { + limit = n + } + } + if limit < 1 { + limit = 1 + } + if limit > 500 { + limit = 500 + } + rows, err := dbq.New(h.pool).ListRandomTracksForUser(r.Context(), + dbq.ListRandomTracksForUserParams{UserID: user.ID, Limit: int32(limit)}) + if err != nil { + h.logger.Error("api: library shuffle", "err", err) + writeErr(w, apierror.InternalMsg("shuffle failed", err)) + return + } + out := make([]TrackRef, 0, len(rows)) + for _, row := range rows { + out = append(out, trackRefFrom(row.Track, row.AlbumTitle, row.ArtistName)) + } + writeJSON(w, http.StatusOK, out) +} + // handleListArtists implements GET /api/artists with two sort modes // (alpha|newest) and enveloped pagination. The alpha branch uses // ListArtistsAlphaWithCovers to ship cover_url + album_count in a diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index 2f522c3a..7e21a3f3 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -255,6 +255,73 @@ func (q *Queries) ListArtistTracksForUser(ctx context.Context, arg ListArtistTra return items, nil } +const listRandomTracksForUser = `-- name: ListRandomTracksForUser :many +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, + albums.title AS album_title, + artists.name AS artist_name +FROM tracks t +JOIN albums ON albums.id = t.album_id +JOIN artists ON artists.id = t.artist_id +WHERE NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) +ORDER BY random() +LIMIT $2 +` + +type ListRandomTracksForUserParams struct { + UserID pgtype.UUID + Limit int32 +} + +type ListRandomTracksForUserRow struct { + Track Track + AlbumTitle string + ArtistName string +} + +// #427 S4: backing query for GET /api/library/shuffle — the online +// source for "Shuffle all". N random tracks across the whole +// library, per-user-quarantine filtered. $1 user_id, $2 limit. +func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTracksForUserParams) ([]ListRandomTracksForUserRow, error) { + rows, err := q.db.Query(ctx, listRandomTracksForUser, arg.UserID, arg.Limit) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListRandomTracksForUserRow + for rows.Next() { + var i ListRandomTracksForUserRow + if err := rows.Scan( + &i.Track.ID, + &i.Track.Title, + &i.Track.AlbumID, + &i.Track.ArtistID, + &i.Track.TrackNumber, + &i.Track.DiscNumber, + &i.Track.DurationMs, + &i.Track.FilePath, + &i.Track.FileSize, + &i.Track.FileFormat, + &i.Track.Bitrate, + &i.Track.Mbid, + &i.Track.Genre, + &i.Track.AddedAt, + &i.Track.UpdatedAt, + &i.AlbumTitle, + &i.ArtistName, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listTracksByAlbum = `-- name: ListTracksByAlbum :many SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at FROM tracks WHERE album_id = $1 diff --git a/internal/db/queries/tracks.sql b/internal/db/queries/tracks.sql index 1e661ce0..4019b684 100644 --- a/internal/db/queries/tracks.sql +++ b/internal/db/queries/tracks.sql @@ -81,6 +81,23 @@ WHERE t.artist_id = $1 ORDER BY albums.release_date NULLS LAST, albums.sort_title, t.disc_number NULLS FIRST, t.track_number NULLS FIRST, t.id; +-- name: ListRandomTracksForUser :many +-- #427 S4: backing query for GET /api/library/shuffle — the online +-- source for "Shuffle all". N random tracks across the whole +-- library, per-user-quarantine filtered. $1 user_id, $2 limit. +SELECT sqlc.embed(t), + albums.title AS album_title, + artists.name AS artist_name +FROM tracks t +JOIN albums ON albums.id = t.album_id +JOIN artists ON artists.id = t.artist_id +WHERE NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) +ORDER BY random() +LIMIT $2; + -- name: CountTracksByArtist :one -- Used by request-progress reporting to count tracks ingested under a -- matched artist (sum across all the artist's albums) while a request From a59967be20a5d456aeb8d7ea86e53c63bbe74ba1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 21:41:48 -0400 Subject: [PATCH 23/27] fix(offline): move Shuffle all from Home to Library only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operator: Shuffle-all belongs in the Library view, not the Home app bar. Moved the shuffle IconButton to LibraryScreen's app bar (same behavior — online server-random / offline cache-union via shuffleSourceProvider); reverted Home's app bar to the original MainAppBarActions-only and dropped the now-unused imports. Co-Authored-By: Claude Opus 4.7 (1M context) --- flutter_client/lib/library/home_screen.dart | 25 +------------------ .../lib/library/library_screen.dart | 23 ++++++++++++++++- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/flutter_client/lib/library/home_screen.dart b/flutter_client/lib/library/home_screen.dart index c736a31f..968ed772 100644 --- a/flutter_client/lib/library/home_screen.dart +++ b/flutter_client/lib/library/home_screen.dart @@ -4,9 +4,7 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:go_router/go_router.dart'; import '../api/errors.dart'; -import '../cache/shuffle_source.dart'; import '../cache/tile_providers.dart'; -import '../player/player_provider.dart'; import '../models/playlist.dart'; import '../models/system_playlists_status.dart'; import '../models/track.dart'; @@ -43,28 +41,7 @@ class HomeScreen extends ConsumerWidget { backgroundColor: fs.obsidian, elevation: 0, title: Text('Minstrel', style: TextStyle(color: fs.parchment)), - actions: [ - IconButton( - key: const Key('shuffle_all_button'), - tooltip: 'Shuffle all', - icon: Icon(Icons.shuffle, color: fs.parchment), - onPressed: () async { - final messenger = ScaffoldMessenger.of(context); - final refs = - await ref.read(shuffleSourceProvider).tracks(); - if (refs.isEmpty) { - messenger.showSnackBar(const SnackBar( - content: Text('Nothing to shuffle yet'), - )); - return; - } - await ref - .read(playerActionsProvider) - .playTracks(refs, shuffle: true); - }, - ), - const MainAppBarActions(currentRoute: '/home'), - ], + actions: const [MainAppBarActions(currentRoute: '/home')], ), body: SafeArea( child: index.when( diff --git a/flutter_client/lib/library/library_screen.dart b/flutter_client/lib/library/library_screen.dart index b3e64948..d6cb3a93 100644 --- a/flutter_client/lib/library/library_screen.dart +++ b/flutter_client/lib/library/library_screen.dart @@ -15,6 +15,7 @@ import '../cache/cache_first.dart'; import '../cache/connectivity_provider.dart'; import '../cache/db.dart'; import '../cache/metadata_prefetcher.dart'; +import '../cache/shuffle_source.dart'; import '../cache/tile_providers.dart'; import '../library/library_providers.dart' show dioProvider; import '../models/album.dart'; @@ -377,7 +378,27 @@ class _LibraryScreenState extends ConsumerState backgroundColor: fs.obsidian, elevation: 0, title: Text('Library', style: TextStyle(color: fs.parchment)), - actions: const [MainAppBarActions(currentRoute: '/library')], + actions: [ + IconButton( + key: const Key('shuffle_all_button'), + tooltip: 'Shuffle all', + icon: Icon(Icons.shuffle, color: fs.parchment), + onPressed: () async { + final messenger = ScaffoldMessenger.of(context); + final refs = await ref.read(shuffleSourceProvider).tracks(); + if (refs.isEmpty) { + messenger.showSnackBar(const SnackBar( + content: Text('Nothing to shuffle yet'), + )); + return; + } + await ref + .read(playerActionsProvider) + .playTracks(refs, shuffle: true); + }, + ), + const MainAppBarActions(currentRoute: '/library'), + ], bottom: TabBar( controller: _ctrl, isScrollable: true, From 035b000bb0580abbbed729ee4fb1209e7da51699 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 21:44:46 -0400 Subject: [PATCH 24/27] fix(analyze): drop unused drift import in shuffle_source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Query-builder methods come through the generated AppDb, not the drift package directly — the import was dead. (#427 S4a) Co-Authored-By: Claude Opus 4.7 (1M context) --- flutter_client/lib/cache/shuffle_source.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/flutter_client/lib/cache/shuffle_source.dart b/flutter_client/lib/cache/shuffle_source.dart index 5188a5fd..45c68919 100644 --- a/flutter_client/lib/cache/shuffle_source.dart +++ b/flutter_client/lib/cache/shuffle_source.dart @@ -10,7 +10,6 @@ import 'dart:math'; -import 'package:drift/drift.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../library/library_providers.dart' show libraryApiProvider; From 2d9775244cd848f5498d951d6877799739eddd36 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 22:12:51 -0400 Subject: [PATCH 25/27] =?UTF-8?q?feat(offline):=20#427=20S4b=20=E2=80=94?= =?UTF-8?q?=20offline=20pools=20on=20Home=20beside=20system=20tiles?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operator's model: offline, surface the cache-backed pools right where the (now play-disabled, S4a) system playlists sit, so Home still has something to play. - ShuffleSource gains recentlyPlayed() (cache by lastPlayedAt desc, liked included) and liked() (cache ∩ liked set); shared _refs() materializes ordered ids from cached_tracks/artists/albums. Shuffle-all reuses the same recency walk. - Home Playlists row: when offlineProvider is true, prepend two tiles — "Recently played" / "Liked" — sized to PlaylistCard. Tap → shuffle+play that pool from cache; empty → snackbar. System tiles still render (play disabled per S4a) beside them. - offline=false (online + all widget tests) → no extra tiles; placeholder-count tests unaffected. Closes the S4 thread of the #427 umbrella (S4a + S4b). Co-Authored-By: Claude Opus 4.7 (1M context) --- flutter_client/lib/cache/shuffle_source.dart | 89 ++++++++++---- flutter_client/lib/library/home_screen.dart | 121 +++++++++++++++++-- 2 files changed, 176 insertions(+), 34 deletions(-) diff --git a/flutter_client/lib/cache/shuffle_source.dart b/flutter_client/lib/cache/shuffle_source.dart index 45c68919..e42d9767 100644 --- a/flutter_client/lib/cache/shuffle_source.dart +++ b/flutter_client/lib/cache/shuffle_source.dart @@ -1,18 +1,22 @@ -// "Shuffle all" source (#427 S4). Always-present; the pool degrades -// gracefully with reachability: +// Offline play sources over the local cache index (#427 S4). +// +// "Shuffle all" is always-present and degrades with reachability: // online → GET /api/library/shuffle (random over the whole library) // offline → a client shuffle over the entire local cache index // -// Offline is a UNION over every cached track regardless of storage -// bucket — liked AND recently-played both included. The two-bucket -// split (S2) is storage/eviction-only and never filters playback, -// which is exactly the property this relies on. +// Offline-only pools surfaced on Home beside the (disabled) system +// playlists: "Recently played" (cache by lastPlayedAt desc) and +// "Liked" (cache ∩ liked set). All of these are UNIONs over the +// cache regardless of storage bucket — liked AND recently-played +// both included. The two-bucket split (S2) is storage/eviction-only +// and never filters playback, which is exactly what this relies on. import 'dart:math'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../library/library_providers.dart' show libraryApiProvider; +import '../likes/likes_provider.dart' show likedIdsProvider; import '../models/track.dart'; import 'adapters.dart'; import 'audio_cache_manager.dart' show appDbProvider; @@ -22,44 +26,77 @@ class ShuffleSource { ShuffleSource(this._ref); final Ref _ref; - /// Returns up to [limit] tracks to play as a shuffle-all. Online: - /// server-random. Offline: every cached track (liked + recent), - /// shuffled client-side. Empty offline → nothing is cached yet. + /// Shuffle-all. Online: server-random. Offline: every cached + /// track, shuffled. Empty offline → nothing cached yet. Future> tracks({int limit = 100}) async { if (_ref.read(offlineProvider)) { - return _offline(limit); + final ids = await _cachedIdsByRecency(); + final list = await _refs(ids); + list.shuffle(Random()); + return list.length > limit ? list.sublist(0, limit) : list; } final api = await _ref.read(libraryApiProvider.future); return api.shuffle(limit: limit); } - Future> _offline(int limit) async { - final db = _ref.read(appDbProvider); - final cachedIds = - (await db.select(db.audioCacheIndex).get()).map((r) => r.trackId).toSet(); - if (cachedIds.isEmpty) return const []; + /// Cached tracks, most-recently-played first (liked included). + /// Cache-only — surfaced on Home only when offline. + Future> recentlyPlayed({int limit = 100}) async { + final ids = await _cachedIdsByRecency(); + final list = await _refs(ids); + return list.length > limit ? list.sublist(0, limit) : list; + } + /// Cached tracks that are in the user's liked set. Cache-only — + /// surfaced on Home only when offline. + Future> liked({int limit = 100}) async { + final likedSet = + _ref.read(likedIdsProvider).value?.tracks ?? const {}; + final ids = + (await _cachedIdsByRecency()).where(likedSet.contains).toList(); + final list = await _refs(ids); + return list.length > limit ? list.sublist(0, limit) : list; + } + + /// Cached track ids ordered by lastPlayedAt desc (nulls last so + /// never-touched downloads sort after real plays). + Future> _cachedIdsByRecency() async { + final db = _ref.read(appDbProvider); + final rows = await db.select(db.audioCacheIndex).get(); + rows.sort((a, b) { + final av = a.lastPlayedAt, bv = b.lastPlayedAt; + if (av == null && bv == null) return 0; + if (av == null) return 1; + if (bv == null) return -1; + return bv.compareTo(av); + }); + return rows.map((r) => r.trackId).toList(); + } + + /// Materializes ordered track ids into TrackRefs from the cached + /// metadata tables, preserving the given order. + Future> _refs(List orderedIds) async { + if (orderedIds.isEmpty) return const []; + final db = _ref.read(appDbProvider); final meta = await (db.select(db.cachedTracks) - ..where((t) => t.id.isIn(cachedIds.toList()))) + ..where((t) => t.id.isIn(orderedIds))) .get(); if (meta.isEmpty) return const []; - + final byId = {for (final t in meta) t.id: t}; final artistName = { for (final a in await db.select(db.cachedArtists).get()) a.id: a.name }; final albumTitle = { for (final a in await db.select(db.cachedAlbums).get()) a.id: a.title }; - - final refs = [ - for (final t in meta) - t.toRef( - artistName: artistName[t.artistId] ?? '', - albumTitle: albumTitle[t.albumId] ?? '', - ) + return [ + for (final id in orderedIds) + if (byId[id] case final t?) + t.toRef( + artistName: artistName[t.artistId] ?? '', + albumTitle: albumTitle[t.albumId] ?? '', + ) ]; - refs.shuffle(Random()); - return refs.length > limit ? refs.sublist(0, limit) : refs; } } diff --git a/flutter_client/lib/library/home_screen.dart b/flutter_client/lib/library/home_screen.dart index 968ed772..a3eff3dc 100644 --- a/flutter_client/lib/library/home_screen.dart +++ b/flutter_client/lib/library/home_screen.dart @@ -4,10 +4,13 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:go_router/go_router.dart'; import '../api/errors.dart'; +import '../cache/offline_provider.dart'; +import '../cache/shuffle_source.dart'; import '../cache/tile_providers.dart'; import '../models/playlist.dart'; import '../models/system_playlists_status.dart'; import '../models/track.dart'; +import '../player/player_provider.dart'; import '../playlists/playlists_provider.dart'; import '../playlists/widgets/playlist_card.dart'; import '../playlists/widgets/playlist_placeholder_card.dart'; @@ -63,6 +66,7 @@ class HomeScreen extends ConsumerWidget { _PlaylistsSection( playlists: allPlaylists.value?.owned ?? const [], status: status.value ?? SystemPlaylistsStatus.empty(), + offline: ref.watch(offlineProvider), ), _RecentlyAddedSection(ids: h.recentlyAddedAlbums), _RediscoverSection( @@ -222,23 +226,124 @@ class _CompactTrackSkeleton extends StatelessWidget { // ─── Sections ──────────────────────────────────────────────────────── class _PlaylistsSection extends StatelessWidget { - const _PlaylistsSection({required this.playlists, required this.status}); + const _PlaylistsSection({ + required this.playlists, + required this.status, + required this.offline, + }); final List playlists; final SystemPlaylistsStatus status; + final bool offline; @override Widget build(BuildContext context) { final items = _buildPlaylistsRow(playlists, status); + final children = [ + // Offline: surface the cache-backed pools where the (now + // play-disabled) system playlists sit, so there's something + // to play. #427 S4b. + if (offline) ...const [ + _OfflinePoolCard( + label: 'Recently played', + icon: Icons.history, + kind: _OfflinePoolKind.recentlyPlayed, + ), + _OfflinePoolCard( + label: 'Liked', + icon: Icons.favorite, + kind: _OfflinePoolKind.liked, + ), + ], + for (final item in items) + if (item is _RealPlaylist) + PlaylistCard(playlist: item.playlist) + else + PlaylistPlaceholderCard( + label: (item as _PlaceholderPlaylist).label, + variant: item.variant, + ), + ]; return HorizontalScrollRow( title: 'Playlists', height: 220, - children: items.map((item) { - if (item is _RealPlaylist) { - return PlaylistCard(playlist: item.playlist); - } - final ph = item as _PlaceholderPlaylist; - return PlaylistPlaceholderCard(label: ph.label, variant: ph.variant); - }).toList(), + children: children, + ); + } +} + +enum _OfflinePoolKind { recentlyPlayed, liked } + +/// Home tile for an offline cache-backed pool. Tapping shuffles + +/// plays that pool from the local cache. Sized to match +/// PlaylistCard so the row stays visually consistent. +class _OfflinePoolCard extends ConsumerWidget { + const _OfflinePoolCard({ + required this.label, + required this.icon, + required this.kind, + }); + final String label; + final IconData icon; + final _OfflinePoolKind kind; + + @override + Widget build(BuildContext context, WidgetRef ref) { + final fs = Theme.of(context).extension()!; + return SizedBox( + width: 176, + child: Material( + color: Colors.transparent, + child: InkWell( + onTap: () async { + final messenger = ScaffoldMessenger.of(context); + final src = ref.read(shuffleSourceProvider); + final refs = await switch (kind) { + _OfflinePoolKind.recentlyPlayed => src.recentlyPlayed(), + _OfflinePoolKind.liked => src.liked(), + }; + if (refs.isEmpty) { + messenger.showSnackBar( + SnackBar(content: Text('No cached $label tracks yet')), + ); + return; + } + await ref + .read(playerActionsProvider) + .playTracks(refs, shuffle: true); + }, + child: Padding( + padding: const EdgeInsets.symmetric(horizontal: 8), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Container( + width: 144, + height: 144, + decoration: BoxDecoration( + color: fs.slate, + borderRadius: BorderRadius.circular(6), + ), + child: Icon(icon, color: fs.accent, size: 56), + ), + const SizedBox(height: 8), + Text( + label, + maxLines: 1, + overflow: TextOverflow.ellipsis, + style: TextStyle(color: fs.parchment, fontSize: 14), + ), + Padding( + padding: const EdgeInsets.only(top: 2), + child: Text( + 'Offline', + style: TextStyle(color: fs.ash, fontSize: 11), + ), + ), + ], + ), + ), + ), + ), ); } } From ce367608199e3edea10c46c3dbb714c128b9d9c9 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 23:03:41 -0400 Subject: [PATCH 26/27] =?UTF-8?q?feat(playlists):=20#417=20=E2=80=94=20"Re?= =?UTF-8?q?freshed=20=E2=80=A6"=20subtitle=20on=20system=20tiles?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last buildable item of the #411 system-playlists-v2 umbrella. System playlists atomic-replace on rebuild, so created_at (already on the wire — no server change) is the last-rotated time. Surface it as a small tile subtitle so users see how fresh a mix is: "Refreshed just now / today / yesterday / N days ago / Mon D". - web PlaylistCard: refreshedLabel() + a muted footer line, shown only when system_variant != null. Unparseable/empty timestamp → suppressed (web test fixtures use created_at:'' so no test churn). - flutter PlaylistCard: mirrored _refreshedLabel() + subtitle under the system badge for isSystem playlists. Friendly wording deliberately distinct from HistoryRow's "m/h ago"; per-surface helper per the project's existing relative-time convention. CI-pending; closes with the umbrella on device-check. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../lib/playlists/widgets/playlist_card.dart | 31 +++++++++++++++++++ web/src/lib/components/PlaylistCard.svelte | 24 ++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/flutter_client/lib/playlists/widgets/playlist_card.dart b/flutter_client/lib/playlists/widgets/playlist_card.dart index cf439056..c7a0a2c5 100644 --- a/flutter_client/lib/playlists/widgets/playlist_card.dart +++ b/flutter_client/lib/playlists/widgets/playlist_card.dart @@ -122,6 +122,16 @@ class PlaylistCard extends ConsumerWidget { ), ), ), + if (playlist.isSystem && _refreshedLabel(playlist.createdAt) != '') + Padding( + padding: const EdgeInsets.only(top: 2), + child: Text( + _refreshedLabel(playlist.createdAt), + maxLines: 1, + overflow: TextOverflow.ellipsis, + style: TextStyle(color: fs.ash, fontSize: 11), + ), + ), ]), ), ), @@ -131,6 +141,27 @@ class PlaylistCard extends ConsumerWidget { String get _refreshLabel => 'Refresh ${playlist.name}'; + /// #417: system playlists atomic-replace on rebuild, so createdAt + /// is the last-rotated time. Friendly wording mirrors the web + /// PlaylistCard. Empty string when the timestamp is unparseable. + String _refreshedLabel(String iso) { + final t = DateTime.tryParse(iso); + if (t == null) return ''; + final now = DateTime.now(); + final mins = now.difference(t).inMinutes; + if (mins < 5) return 'Refreshed just now'; + final startOfToday = DateTime(now.year, now.month, now.day); + if (!t.isBefore(startOfToday)) return 'Refreshed today'; + final days = startOfToday.difference(DateTime(t.year, t.month, t.day)).inDays; + if (days <= 1) return 'Refreshed yesterday'; + if (days < 7) return 'Refreshed $days days ago'; + const months = [ + 'Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', + 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec', + ]; + return 'Refreshed ${months[t.month - 1]} ${t.day}'; + } + /// Forces a server-side rebuild of this system playlist, then /// invalidates the aggregate list so the rotated UUID + new tracks /// land on the next read. ScaffoldMessenger is captured before the diff --git a/web/src/lib/components/PlaylistCard.svelte b/web/src/lib/components/PlaylistCard.svelte index 1436afca..cd401158 100644 --- a/web/src/lib/components/PlaylistCard.svelte +++ b/web/src/lib/components/PlaylistCard.svelte @@ -17,6 +17,27 @@ // (#411 R2) — no hardcoded kind list here. const refreshable = $derived(playlist.refreshable === true); + // #417: system playlists atomic-replace on rebuild, so created_at + // is the last-rotated time. Surface it so users know how fresh the + // mix is. Friendly wording, not the HistoryRow "m/h ago" style. + function refreshedLabel(iso: string): string { + const t = new Date(iso); + if (Number.isNaN(t.getTime())) return ''; + const now = new Date(); + const mins = Math.floor((now.getTime() - t.getTime()) / 60000); + if (mins < 5) return 'Refreshed just now'; + const startOfToday = new Date(now); + startOfToday.setHours(0, 0, 0, 0); + const days = Math.floor((startOfToday.getTime() - t.getTime()) / 86400000) + 1; + if (t.getTime() >= startOfToday.getTime()) return 'Refreshed today'; + if (days <= 1) return 'Refreshed yesterday'; + if (days < 7) return `Refreshed ${days} days ago`; + return `Refreshed ${t.toLocaleDateString(undefined, { month: 'short', day: 'numeric' })}`; + } + const refreshed = $derived( + playlist.system_variant != null ? refreshedLabel(playlist.created_at) : '' + ); + const queryClient = useQueryClient(); let starting = $state(false); @@ -153,6 +174,9 @@ · by {playlist.owner_username} {/if}
+ {#if refreshed} +
{refreshed}
+ {/if} From 0c2b86e736d0201aec126ede061cb729bbe16a94 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 23:15:10 -0400 Subject: [PATCH 27/27] chore(flutter): bump version to 2026.5.15+6 for v2026.05.15.0 Co-Authored-By: Claude Opus 4.7 (1M context) --- flutter_client/pubspec.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flutter_client/pubspec.yaml b/flutter_client/pubspec.yaml index 4a03b811..03c19c18 100644 --- a/flutter_client/pubspec.yaml +++ b/flutter_client/pubspec.yaml @@ -1,7 +1,7 @@ name: minstrel description: Minstrel mobile client publish_to: 'none' -version: 2026.5.14+5 +version: 2026.5.15+6 environment: sdk: '>=3.5.0 <4.0.0'