From 0fdcbc3cbd0ee2cccf8870f311ef867d7bff8b13 Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Tue, 6 Oct 2026 22:00:14 +0700 Subject: [PATCH] fix(playlists): prevent overlapping picker submissions --- lib/widgets/playlist_picker_sheet.dart | 42 +++++++-- test/mornye_collection_sheets_test.dart | 118 ++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 10 deletions(-) diff --git a/lib/widgets/playlist_picker_sheet.dart b/lib/widgets/playlist_picker_sheet.dart index 1a237cee..2d0dde89 100644 --- a/lib/widgets/playlist_picker_sheet.dart +++ b/lib/widgets/playlist_picker_sheet.dart @@ -7,12 +7,15 @@ import 'package:spotiflac_android/l10n/l10n.dart'; import 'package:spotiflac_android/models/track.dart'; import 'package:spotiflac_android/providers/library_collections_provider.dart'; import 'package:spotiflac_android/theme/mornye_theme.dart'; +import 'package:spotiflac_android/utils/logger.dart'; import 'package:spotiflac_android/widgets/app_action_button.dart'; import 'package:spotiflac_android/widgets/app_alert_dialog.dart'; import 'package:spotiflac_android/widgets/app_bottom_sheet.dart'; import 'package:spotiflac_android/widgets/cached_cover_image.dart'; import 'package:spotiflac_android/widgets/mornye_chrome.dart'; +final _log = AppLogger('PlaylistPicker'); + Future showAddTrackToPlaylistSheet( BuildContext context, WidgetRef ref, @@ -68,6 +71,7 @@ class _PlaylistPickerSheetContentState late final PlaylistPickerSummaryRequest _summaryRequest; final Set _selectedPlaylistIds = {}; final Set _committedPlaylistIds = {}; + bool _isSubmitting = false; @override void initState() { @@ -75,7 +79,9 @@ class _PlaylistPickerSheetContentState _summaryRequest = PlaylistPickerSummaryRequest.fromTracks(widget.tracks); } - void _handleDone(List playlists) async { + Future _handleDone(List playlists) async { + if (_isSubmitting) return; + setState(() => _isSubmitting = true); final notifier = ref.read(libraryCollectionsProvider.notifier); final effectiveDisabledIds = { ..._committedPlaylistIds, @@ -88,12 +94,23 @@ class _PlaylistPickerSheetContentState }; final addedNames = []; - for (final playlistId in idsToAdd) { - final playlistName = playlistNamesById[playlistId]; - if (playlistName != null && playlistName.isNotEmpty) { - addedNames.add(playlistName); + try { + for (final playlistId in idsToAdd) { + await notifier.addTracksToPlaylist(playlistId, widget.tracks); + _committedPlaylistIds.add(playlistId); + final playlistName = playlistNamesById[playlistId]; + if (playlistName != null && playlistName.isNotEmpty) { + addedNames.add(playlistName); + } } - await notifier.addTracksToPlaylist(playlistId, widget.tracks); + } catch (error, stack) { + _log.e('Failed to add tracks to selected playlists', error, stack); + if (!mounted) return; + setState(() => _isSubmitting = false); + ScaffoldMessenger.of( + context, + ).showSnackBar(SnackBar(content: Text(context.friendlyError(error)))); + return; } if (!mounted) return; @@ -134,9 +151,11 @@ class _PlaylistPickerSheetContentState final hasNewSelections = idsToAdd.isNotEmpty; void onDone() { + if (_isSubmitting) return; if (hasNewSelections) { _handleDone(resolvedPlaylists); } else { + setState(() => _isSubmitting = true); Navigator.of(context).pop(); } } @@ -187,7 +206,9 @@ class _PlaylistPickerSheetContentState color: Theme.of(context).colorScheme.primary, ), title: Text(context.l10n.collectionCreatePlaylist), + enabled: !_isSubmitting, onTap: () async { + if (_isSubmitting) return; final name = await _promptPlaylistName( context, widget.playlistNamePrefill, @@ -250,10 +271,11 @@ class _PlaylistPickerSheetContentState playlist.trackCount, ), ), - enabled: !isAlreadyIn, + enabled: !isAlreadyIn && !_isSubmitting, selected: isSelected, - onTap: !isAlreadyIn + onTap: !isAlreadyIn && !_isSubmitting ? () { + if (_isSubmitting) return; setState(() { if (_selectedPlaylistIds.contains( playlist.id, @@ -295,12 +317,12 @@ class _PlaylistPickerSheetContentState width: double.infinity, child: context.isMornye ? AppActionButton( - onPressed: onDone, + onPressed: _isSubmitting ? null : onDone, icon: const Icon(CupertinoIcons.checkmark), label: Text(context.l10n.dialogDone), ) : FilledButton( - onPressed: onDone, + onPressed: _isSubmitting ? null : onDone, child: Text(context.l10n.dialogDone), ), ), diff --git a/test/mornye_collection_sheets_test.dart b/test/mornye_collection_sheets_test.dart index 7e9e75e9..715f7b78 100644 --- a/test/mornye_collection_sheets_test.dart +++ b/test/mornye_collection_sheets_test.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/cupertino.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; @@ -10,6 +12,7 @@ import 'package:spotiflac_android/models/track.dart'; import 'package:spotiflac_android/providers/library_collections_provider.dart'; import 'package:spotiflac_android/providers/settings_provider.dart'; import 'package:spotiflac_android/theme/mornye_theme.dart'; +import 'package:spotiflac_android/widgets/app_action_button.dart'; import 'package:spotiflac_android/widgets/app_alert_dialog.dart'; import 'package:spotiflac_android/widgets/mornye_chrome.dart'; import 'package:spotiflac_android/widgets/playlist_picker_sheet.dart'; @@ -32,6 +35,7 @@ class _Collections extends LibraryCollectionsNotifier { List _summaries = []; final additions = []; final createdNames = []; + final pendingAdditions = >[]; @override LibraryCollectionsState build() => LibraryCollectionsState(isLoaded: true); @@ -50,6 +54,9 @@ class _Collections extends LibraryCollectionsNotifier { Iterable tracks, ) async { additions.add(playlistId); + if (pendingAdditions.isNotEmpty) { + await pendingAdditions.removeAt(0).future; + } return PlaylistAddBatchResult( addedCount: tracks.length, alreadyInPlaylistCount: 0, @@ -57,6 +64,19 @@ class _Collections extends LibraryCollectionsNotifier { } } +class _PopObserver extends NavigatorObserver { + int pops = 0; + + @override + void didPop(Route route, Route? previousRoute) { + pops++; + } +} + +VoidCallback? _doneAction(WidgetTester tester, bool mornye) => mornye + ? tester.widget(find.byType(AppActionButton)).onPressed + : tester.widget(find.byType(FilledButton)).onPressed; + PlaylistPickerSummary _playlist(String name, {bool contains = false}) => PlaylistPickerSummary( id: name, @@ -73,6 +93,7 @@ Widget _app( required Brightness brightness, bool mornye = true, double textScale = 1, + List navigatorObservers = const [], }) => ProviderScope( overrides: [ settingsProvider.overrideWith(_Settings.new), @@ -82,6 +103,7 @@ Widget _app( ), ], child: MaterialApp( + navigatorObservers: navigatorObservers, theme: mornye ? MornyeTheme.build(brightness) : ThemeData(), localizationsDelegates: AppLocalizations.localizationsDelegates, supportedLocales: AppLocalizations.supportedLocales, @@ -157,6 +179,102 @@ void main() { } for (final mornye in [true, false]) { + testWidgets('picker submits and closes once (Mornye: $mornye)', ( + tester, + ) async { + final addition = Completer(); + final collections = _Collections() + .._summaries = [_playlist('Road trip')] + ..pendingAdditions.add(addition); + final observer = _PopObserver(); + await tester.pumpWidget( + _app( + Consumer( + builder: (context, ref, _) => TextButton( + onPressed: () => + showAddTracksToPlaylistSheet(context, ref, [_track]), + child: const Text('Open'), + ), + ), + collections, + brightness: Brightness.light, + mornye: mornye, + navigatorObservers: [observer], + ), + ); + await tester.tap(find.text('Open')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Road trip')); + await tester.pump(); + final submit = _doneAction(tester, mornye)!; + // A repeated callback can arrive before the disabled button is rebuilt. + submit(); + submit(); + await tester.pump(); + expect(collections.additions, ['Road trip']); + expect(_doneAction(tester, mornye), isNull); + expect(observer.pops, 0); + + addition.complete(); + await tester.pump(); + // The route remains mounted while its closing animation runs. + submit(); + await tester.pumpAndSettle(); + expect(collections.additions, ['Road trip']); + expect(observer.pops, 1); + expect(find.text('Open'), findsOneWidget); + expect(tester.takeException(), isNull); + }); + + testWidgets('picker retains selections after failure (Mornye: $mornye)', ( + tester, + ) async { + final firstAddition = Completer()..complete(); + final secondAddition = Completer(); + final collections = _Collections() + .._summaries = [_playlist('Road trip'), _playlist('Evening music')] + ..pendingAdditions.addAll([firstAddition, secondAddition]); + final observer = _PopObserver(); + await tester.pumpWidget( + _app( + Consumer( + builder: (context, ref, _) => TextButton( + onPressed: () => + showAddTracksToPlaylistSheet(context, ref, [_track]), + child: const Text('Open'), + ), + ), + collections, + brightness: Brightness.light, + mornye: mornye, + navigatorObservers: [observer], + ), + ); + await tester.tap(find.text('Open')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Road trip')); + await tester.tap(find.text('Evening music')); + await tester.pump(); + await tester.tap(find.text('Done')); + await tester.pump(); + expect(collections.additions, ['Road trip', 'Evening music']); + secondAddition.completeError(StateError('write failed')); + await tester.pump(); + expect(observer.pops, 0); + expect(_doneAction(tester, mornye), isNotNull); + + // Retrying keeps the failed selection and skips the persisted playlist. + await tester.tap(find.text('Done')); + await tester.pumpAndSettle(); + expect(collections.additions, [ + 'Road trip', + 'Evening music', + 'Evening music', + ]); + expect(observer.pops, 1); + expect(tester.takeException(), isNull); + }); + testWidgets('picker adds only new selections (Mornye: $mornye)', ( tester, ) async {