From 1ef81b453013e4c23df2ef4cc0d9125b2664dc67 Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Thu, 17 Sep 2026 17:32:12 +0700 Subject: [PATCH] perf(library): batch album deletion and refresh once --- lib/providers/local_library_provider.dart | 7 +- lib/screens/local_album_screen.dart | 2 +- lib/services/library_cleanup.dart | 26 ++++ lib/services/library_database.dart | 15 +- lib/utils/confirm_and_delete_tracks.dart | 16 +- test/library_batch_delete_test.dart | 172 ++++++++++++++++++++++ 6 files changed, 224 insertions(+), 14 deletions(-) create mode 100644 test/library_batch_delete_test.dart diff --git a/lib/providers/local_library_provider.dart b/lib/providers/local_library_provider.dart index 1af11c99..5e266b2e 100644 --- a/lib/providers/local_library_provider.dart +++ b/lib/providers/local_library_provider.dart @@ -1222,7 +1222,12 @@ class LocalLibraryNotifier extends Notifier { } Future removeItem(String id) async { - await _db.delete(id); + await removeItems([id]); + } + + Future removeItems(Iterable ids) async { + if (ids.isEmpty) return; + await _db.deleteByIds(ids); await _refreshSummaryFromStorage(); } diff --git a/lib/screens/local_album_screen.dart b/lib/screens/local_album_screen.dart index 7b3245c0..9c19a6ac 100644 --- a/lib/screens/local_album_screen.dart +++ b/lib/screens/local_album_screen.dart @@ -129,9 +129,9 @@ class _LocalAlbumScreenState extends ConsumerState final deleted = await deleteFile(item.filePath); if (!deleted) return false; } - await libraryNotifier.removeItem(id); return true; }, + persistDeletedItems: libraryNotifier.removeItems, onExitSelectionMode: exitSelectionMode, ); diff --git a/lib/services/library_cleanup.dart b/lib/services/library_cleanup.dart index 1f798d16..65c1be4f 100644 --- a/lib/services/library_cleanup.dart +++ b/lib/services/library_cleanup.dart @@ -3,6 +3,32 @@ import 'package:sqflite/sqflite.dart'; import 'package:spotiflac_android/utils/file_access.dart'; import 'package:spotiflac_android/utils/logger.dart'; +/// Deletes a selection atomically without exceeding SQLite's parameter budget. +Future deleteLibraryItemsByIds(Database db, Iterable ids) async { + final uniqueIds = ids.toSet().toList(); + if (uniqueIds.isEmpty) return; + await db.transaction((txn) async { + const chunkSize = 500; + for (var start = 0; start < uniqueIds.length; start += chunkSize) { + final chunk = uniqueIds.sublist( + start, + (start + chunkSize).clamp(0, uniqueIds.length), + ); + final placeholders = List.filled(chunk.length, '?').join(','); + await txn.delete( + 'library_path_keys', + where: 'item_id IN ($placeholders)', + whereArgs: chunk, + ); + await txn.delete( + 'library', + where: 'id IN ($placeholders)', + whereArgs: chunk, + ); + } + }); +} + Future pruneUnreferencedLibraryCovers( Directory directory, Set referencedPaths, diff --git a/lib/services/library_database.dart b/lib/services/library_database.dart index e9f62ee0..200c0959 100644 --- a/lib/services/library_database.dart +++ b/lib/services/library_database.dart @@ -1986,15 +1986,12 @@ class LibraryDatabase { } Future delete(String id) async { - final db = await database; - await db.transaction((txn) async { - await txn.delete( - 'library_path_keys', - where: 'item_id = ?', - whereArgs: [id], - ); - await txn.delete('library', where: 'id = ?', whereArgs: [id]); - }); + await deleteByIds([id]); + } + + Future deleteByIds(Iterable ids) async { + if (ids.isEmpty) return; + await deleteLibraryItemsByIds(await database, ids); } Future cleanupMissingFiles({ diff --git a/lib/utils/confirm_and_delete_tracks.dart b/lib/utils/confirm_and_delete_tracks.dart index 1cca1a92..ca233d6d 100644 --- a/lib/utils/confirm_and_delete_tracks.dart +++ b/lib/utils/confirm_and_delete_tracks.dart @@ -8,10 +8,13 @@ import 'package:spotiflac_android/l10n/l10n.dart'; /// /// Returns the number of items [deleteItem] reported deleted, or null if the /// user cancelled the dialog. +/// [persistDeletedItems] commits successfully deleted IDs once before the UI +/// reports completion, including partial success if a later deletion throws. Future confirmAndDeleteTracks({ required BuildContext context, required List ids, required Future Function(String id) deleteItem, + Future Function(List ids)? persistDeletedItems, required VoidCallback onExitSelectionMode, }) async { final confirmed = await showDialog( @@ -37,10 +40,17 @@ Future confirmAndDeleteTracks({ if (confirmed != true || !context.mounted) return null; - var deletedCount = 0; - for (final id in ids) { - if (await deleteItem(id)) deletedCount++; + final deletedIds = []; + try { + for (final id in ids) { + if (await deleteItem(id)) deletedIds.add(id); + } + } finally { + if (deletedIds.isNotEmpty && persistDeletedItems != null) { + await persistDeletedItems(deletedIds); + } } + final deletedCount = deletedIds.length; onExitSelectionMode(); diff --git a/test/library_batch_delete_test.dart b/test/library_batch_delete_test.dart new file mode 100644 index 00000000..71038730 --- /dev/null +++ b/test/library_batch_delete_test.dart @@ -0,0 +1,172 @@ +import 'dart:async'; + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:spotiflac_android/l10n/app_localizations.dart'; +import 'package:spotiflac_android/services/library_cleanup.dart'; +import 'package:spotiflac_android/utils/confirm_and_delete_tracks.dart'; +import 'package:sqflite/sqflite.dart'; + +class _DeleteDatabase implements Database, Transaction { + final rows = {}; + final pathKeys = {}; + final chunks = >[]; + int transactions = 0; + bool failRows = false; + + @override + Future transaction( + Future Function(Transaction txn) action, { + bool? exclusive, + }) async { + transactions++; + final previousRows = Set.of(rows); + final previousKeys = Set.of(pathKeys); + try { + return await action(this); + } catch (_) { + rows + ..clear() + ..addAll(previousRows); + pathKeys + ..clear() + ..addAll(previousKeys); + rethrow; + } + } + + @override + Future delete( + String table, { + String? where, + List? whereArgs, + }) async { + expect(whereArgs!.length, lessThanOrEqualTo(500)); + expect('?'.allMatches(where!).length, whereArgs.length); + expect(table, isIn(['library_path_keys', 'library'])); + if (table == 'library_path_keys') { + expect(where, startsWith('item_id IN (')); + chunks.add(List.of(whereArgs)); + pathKeys.removeAll(whereArgs); + } else { + expect(where, startsWith('id IN (')); + expect(whereArgs, chunks.last); + if (failRows) throw StateError('write failed'); + rows.removeAll(whereArgs); + } + return whereArgs.length; + } + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +void main() { + test('large selection deletes paired rows in bounded chunks once', () async { + final ids = List.generate(1205, (index) => 'track-$index'); + final db = _DeleteDatabase() + ..rows.addAll([...ids, 'retained']) + ..pathKeys.addAll([...ids, 'retained']); + await deleteLibraryItemsByIds(db, [...ids, ...ids.take(10)]); + expect(db.transactions, 1); + expect(db.chunks.map((chunk) => chunk.length), [500, 500, 205]); + expect(db.chunks.expand((chunk) => chunk), ids); + expect(db.rows, {'retained'}); + expect(db.pathKeys, {'retained'}); + }); + + test('empty selection skips storage; failure keeps rows and keys', () async { + final db = _DeleteDatabase() + ..rows.add('retained') + ..pathKeys.add('retained'); + await deleteLibraryItemsByIds(db, []); + expect(db.transactions, 0); + db.failRows = true; + await expectLater( + deleteLibraryItemsByIds(db, ['retained']), + throwsStateError, + ); + expect(db.rows, {'retained'}); + expect(db.pathKeys, {'retained'}); + }); + + for (final mode in ['partial', 'cancel', 'throw', 'none']) { + testWidgets('batch confirmation persists successful IDs: $mode', ( + tester, + ) async { + final events = []; + final persisted = >[]; + final commit = Completer(); + int? result; + Object? failure; + await tester.pumpWidget( + MaterialApp( + localizationsDelegates: AppLocalizations.localizationsDelegates, + supportedLocales: AppLocalizations.supportedLocales, + home: Scaffold( + body: Builder( + builder: (context) => TextButton( + onPressed: () async { + try { + result = await confirmAndDeleteTracks( + context: context, + ids: ['first', 'failed', 'last'], + deleteItem: (id) async { + events.add(id); + if (mode == 'throw' && id == 'last') { + throw StateError('file delete failed'); + } + return mode != 'none' && id != 'failed'; + }, + persistDeletedItems: (ids) async { + persisted.add(List.of(ids)); + await commit.future; + events.add('committed'); + }, + onExitSelectionMode: () => events.add('exit'), + ); + } catch (error) { + failure = error; + } + }, + child: const Text('Start'), + ), + ), + ), + ), + ); + await tester.tap(find.text('Start')); + await tester.pumpAndSettle(); + if (mode == 'cancel') { + await tester.tap(find.widgetWithText(TextButton, 'Cancel')); + } else { + await tester.tap(find.byType(FilledButton)); + } + await tester.pumpAndSettle(); + if (mode == 'cancel' || mode == 'none') { + expect(persisted, isEmpty); + } else { + expect(persisted, [ + mode == 'throw' ? ['first'] : ['first', 'last'], + ]); + expect(events, isNot(contains('exit'))); + } + commit.complete(); + await tester.pumpAndSettle(); + if (mode == 'throw') { + expect(failure, isStateError); + expect(events.last, 'committed'); + expect(events, isNot(contains('exit'))); + } else { + expect(failure, isNull); + expect(result, mode == 'cancel' ? null : (mode == 'none' ? 0 : 2)); + if (mode == 'cancel') { + expect(events, isEmpty); + } else { + expect(events.last, 'exit'); + } + } + expect(tester.takeException(), isNull); + }); + } +}