From ae679214a960c1118f643e98c28b909ddea0ba1f Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Fri, 2 Oct 2026 14:54:47 +0700 Subject: [PATCH] fix(lyrics): preserve batch track identity and report results promptly --- lib/services/batch_metadata_re_enrich.dart | 14 ++++- lib/services/local_track_batch_actions.dart | 46 ++++++++++++++--- test/batch_metadata_re_enrich_test.dart | 14 +++++ test/downloaded_album_reenrich_test.dart | 3 ++ test/local_track_batch_actions_test.dart | 57 +++++++++++++++++++++ 5 files changed, 125 insertions(+), 9 deletions(-) diff --git a/lib/services/batch_metadata_re_enrich.dart b/lib/services/batch_metadata_re_enrich.dart index 0b92c935..ab9472a1 100644 --- a/lib/services/batch_metadata_re_enrich.dart +++ b/lib/services/batch_metadata_re_enrich.dart @@ -107,6 +107,7 @@ Map buildBatchReEnrichRequest({ required List updateFields, bool previewOnly = false, Map? resolvedMetadata, + String? sourceTrackId, }) { final request = { 'file_path': item.filePath, @@ -115,7 +116,7 @@ Map buildBatchReEnrichRequest({ 'embed_lyrics': settings.embedLyrics, 'lyrics_mode': settings.lyricsMode, 'artist_tag_mode': settings.artistTagMode, - 'spotify_id': '', + 'spotify_id': sourceTrackId ?? '', 'track_name': item.trackName, 'artist_name': item.artistName, 'album_name': item.albumName, @@ -131,7 +132,12 @@ Map buildBatchReEnrichRequest({ 'label': item.label ?? '', 'copyright': item.copyright ?? '', 'duration_ms': (item.duration ?? 0) * 1000, - 'search_online': resolvedMetadata == null, + // Lyrics-only previews do not need a second metadata-provider search. + // Preserve the file's title/artist and fetch lyrics during apply. + 'search_online': + resolvedMetadata == null && + !(updateFields.length == 1 && + updateFields.single == ReEnrichFields.lyrics), 'replace_release_metadata': false, 'update_fields': updateFields, if (previewOnly) 'preview_only': true, @@ -139,6 +145,10 @@ Map buildBatchReEnrichRequest({ if (resolvedMetadata != null) { request.addAll(resolvedMetadata); } + if ((request['spotify_id']?.toString().trim().isEmpty ?? true) && + sourceTrackId != null) { + request['spotify_id'] = sourceTrackId; + } return request; } diff --git a/lib/services/local_track_batch_actions.dart b/lib/services/local_track_batch_actions.dart index dc0a3c1e..f6eca632 100644 --- a/lib/services/local_track_batch_actions.dart +++ b/lib/services/local_track_batch_actions.dart @@ -19,11 +19,14 @@ import 'package:spotiflac_android/utils/ffmpeg_reenrich.dart'; import 'package:spotiflac_android/utils/file_access.dart'; import 'package:spotiflac_android/utils/int_utils.dart'; import 'package:spotiflac_android/utils/lyrics_metadata_helper.dart'; +import 'package:spotiflac_android/utils/logger.dart'; import 'package:spotiflac_android/utils/string_utils.dart'; import 'package:spotiflac_android/widgets/batch_progress_dialog.dart'; import 'package:spotiflac_android/widgets/re_enrich_field_dialog.dart'; import 'package:spotiflac_android/widgets/re_enrich_review_sheet.dart'; +final _reEnrichLog = AppLogger('BatchReEnrich'); + Future queueLocalTracksAsFlac( BuildContext context, WidgetRef ref, @@ -176,6 +179,11 @@ Future reEnrichLibraryTracks( onSelectionHide: onSelectionHide, onSelectionRestore: onSelectionRestore, onComplete: onComplete, + sourceTrackIds: { + for (final track in downloads) + if (track.spotifyId?.trim().isNotEmpty == true) + track.id: track.spotifyId!, + }, refreshLibrary: () async { // Downloaded tracks read history, not the local-scan index. Read back // saved tags so failed or unselected fields cannot change history. @@ -245,6 +253,7 @@ Future reEnrichLocalTracks( required VoidCallback onSelectionRestore, required VoidCallback onComplete, Future Function()? refreshLibrary, + Map sourceTrackIds = const {}, }) async { if (selected.isEmpty) return; // Capture a stable route context before a caller removes its overlay. @@ -263,6 +272,7 @@ Future reEnrichLocalTracks( } final runner = BatchReEnrichRunner( + sourceTrackIds: sourceTrackIds, beginPhase: () async { final settings = ref.read(settingsProvider); await ref @@ -346,6 +356,12 @@ Future reEnrichLocalTracks( if (!context.mounted || !isActive()) return; if (!cancelled) BatchProgressDialog.dismiss(context); + // Report file-write outcomes immediately. A scan of a large Library can + // take much longer than the batch and must not hide its completed result. + ScaffoldMessenger.of(context).clearSnackBars(); + final summary = lyricsSummary.message(context.l10n, successCount, total); + final resultClosed = showReEnrichResultDialog(context, message: summary); + if (refreshLibrary != null) { await refreshLibrary(); } else { @@ -354,16 +370,14 @@ Future reEnrichLocalTracks( if (!context.mounted || !isActive()) return; onComplete(); - - ScaffoldMessenger.of(context).clearSnackBars(); - final summary = lyricsSummary.message(context.l10n, successCount, total); - await showReEnrichResultDialog(context, message: summary); + await resultClosed; } /// Runs a batch against one settings snapshot per phase. Dependencies are /// injectable so cancellation, dispatch, and partial failure can be verified /// without writing media files or opening dialogs. class BatchReEnrichRunner { + final Map sourceTrackIds; final Future Function() beginPhase; final Future> Function(Map) reEnrich; final Future Function({ @@ -380,6 +394,7 @@ class BatchReEnrichRunner { BatchReEnrichRunner({ required this.beginPhase, + this.sourceTrackIds = const {}, this.reEnrich = PlatformBridge.reEnrichFile, this.writeSidecar = writeReEnrichSidecarLrc, this.applyFfmpeg = applyFfmpegReEnrichResult, @@ -412,10 +427,16 @@ class BatchReEnrichRunner { settings: settings, updateFields: fields, previewOnly: true, + sourceTrackId: sourceTrackIds[item.id], ), ); final rawMetadata = result['enriched_metadata']; - if (result['method'] != 'preview' || rawMetadata is! Map) continue; + if (result['method'] != 'preview' || rawMetadata is! Map) { + _reEnrichLog.w( + 'Metadata lookup failed for ${item.id}: ${result['error'] ?? result['method']}', + ); + continue; + } final metadata = rawMetadata.map( (key, value) => MapEntry(key.toString(), value), ); @@ -429,7 +450,9 @@ class BatchReEnrichRunner { changes: changes, ), ); - } catch (_) { + } catch (error, stack) { + _reEnrichLog.w('Metadata lookup failed for ${item.id}: $error'); + _reEnrichLog.d('$stack'); // A failed lookup must not prevent review of other tracks. } } @@ -457,6 +480,7 @@ class BatchReEnrichRunner { settings: settings, updateFields: preview.updateFields, resolvedMetadata: preview.enrichedMetadata, + sourceTrackId: sourceTrackIds[preview.item.id], ), ); switch (result['method']) { @@ -475,9 +499,17 @@ class BatchReEnrichRunner { )) { successes++; onResult?.call(result); + } else { + _reEnrichLog.w('Metadata write failed for ${preview.item.id}'); } + default: + _reEnrichLog.w( + 'Re-enrich failed for ${preview.item.id}: ${result['error'] ?? result['method']}', + ); } - } catch (_) { + } catch (error, stack) { + _reEnrichLog.w('Re-enrich failed for ${preview.item.id}: $error'); + _reEnrichLog.d('$stack'); // Keep successful files and continue with the rest of the selection. } } diff --git a/test/batch_metadata_re_enrich_test.dart b/test/batch_metadata_re_enrich_test.dart index fc29234b..7574d9e0 100644 --- a/test/batch_metadata_re_enrich_test.dart +++ b/test/batch_metadata_re_enrich_test.dart @@ -112,6 +112,7 @@ void main() { item: _item(), settings: const AppSettings(embeddedCoverMaxDimension: 1000), updateFields: const ['isrc'], + sourceTrackId: 'original-id', resolvedMetadata: const { 'isrc': 'USRC17607839', 'spotify_id': 'resolved-id', @@ -125,6 +126,19 @@ void main() { expect(request['cover_max_dimension'], 1000); }); + test('empty preview identity cannot discard the original track ID', () { + final request = buildBatchReEnrichRequest( + item: _item(), + settings: const AppSettings(), + updateFields: const [ReEnrichFields.lyrics], + sourceTrackId: 'original-id', + resolvedMetadata: const {'spotify_id': ''}, + ); + + expect(request['spotify_id'], 'original-id'); + expect(request['search_online'], isFalse); + }); + test('review only includes values that would actually change', () { final changes = buildReEnrichMetadataChanges( _item(isrc: 'OLD'), diff --git a/test/downloaded_album_reenrich_test.dart b/test/downloaded_album_reenrich_test.dart index ecb10d21..5557bc91 100644 --- a/test/downloaded_album_reenrich_test.dart +++ b/test/downloaded_album_reenrich_test.dart @@ -149,6 +149,7 @@ void main() { UnifiedLibraryItem.fromDownloadHistory( DownloadHistoryItem( id: 'download', + spotifyId: 'provider-a:track-1', trackName: 'Downloaded', artistName: 'Artist', albumName: 'Album', @@ -186,6 +187,7 @@ void main() { await tester.tap(find.text('Review changes')); await tester.pumpAndSettle(); expect(requests.map((request) => request['file_path']), paths); + expect(requests.first['spotify_id'], 'provider-a:track-1'); await tester.tap(find.text('Apply changes')); for (var attempt = 0; attempt < 50 && !completed; attempt++) { await tester.pump(const Duration(milliseconds: 100)); @@ -198,6 +200,7 @@ void main() { ...paths, ...paths, ]); + expect(requests[paths.length]['spotify_id'], 'provider-a:track-1'); expect(history._updates, {'download': 'USABC2600001'}); expect(library._refreshes, includeLocal ? 1 : 0); expect(completed, isTrue); diff --git a/test/local_track_batch_actions_test.dart b/test/local_track_batch_actions_test.dart index 03edd5be..d652f770 100644 --- a/test/local_track_batch_actions_test.dart +++ b/test/local_track_batch_actions_test.dart @@ -48,6 +48,58 @@ class _BatchLibrary extends LocalLibraryNotifier { } void main() { + test( + 'lyrics-only batch retains provider identity without metadata lookups', + () async { + final requests = >[]; + final runner = BatchReEnrichRunner( + beginPhase: () async => const AppSettings(embedLyrics: true), + sourceTrackIds: {'1': 'provider-a:track-1'}, + reEnrich: (request) async { + requests.add(request); + return request['preview_only'] == true + ? { + 'method': 'preview', + 'enriched_metadata': { + 'spotify_id': request['spotify_id'], + 'duration_ms': request['duration_ms'], + }, + } + : {'method': 'native', 'lyrics_status': 'updated'}; + }, + writeSidecar: + ({required audioFilePath, required reEnrichResult}) async {}, + ); + final previews = await runner.preview( + [item('1')], + const ReEnrichFieldSelection( + mode: ReEnrichBatchMode.selectedFields, + fields: [ReEnrichFields.lyrics], + ), + shouldStop: running, + onProgress: progress, + ); + expect(previews.single.updateFields, ['lyrics']); + expect( + await runner.apply(previews, shouldStop: running, onProgress: progress), + 1, + ); + expect(requests, hasLength(2)); + expect( + requests.map((request) => request['spotify_id']), + everyElement('provider-a:track-1'), + ); + expect( + requests.map((request) => request['search_online']), + everyElement(false), + ); + expect( + requests.map((request) => request['track_name']), + everyElement('Song 1'), + ); + }, + ); + test( 'lookup/apply each capture settings once, dispatch native/ffmpeg, and retain partial success', () async { @@ -305,6 +357,11 @@ void main() { await tester.tap(find.text('Apply changes')); await tester.pumpAndSettle(); expect(library._refreshStarted, isTrue); + // The result is visible even while an entire-library scan is blocked. + expect( + find.text('Metadata re-enriched successfully (2/2)'), + findsOneWidget, + ); expect(settings.phases.length, 2); active = !leaveDuringRefresh; library.refreshed.complete();