From 5856729e9af04db08f1c79c4f0798597e2c61288 Mon Sep 17 00:00:00 2001 From: zarzet Date: Thu, 23 Jul 2026 18:00:22 +0700 Subject: [PATCH] fix(library): persist records before download completion --- lib/providers/download_history_provider.dart | 35 ++++ lib/providers/download_queue_provider.dart | 123 +++++++------ ...download_queue_provider_native_worker.dart | 168 ++++++++++-------- test/download_history_logic_test.dart | 73 +++++++- 4 files changed, 264 insertions(+), 135 deletions(-) diff --git a/lib/providers/download_history_provider.dart b/lib/providers/download_history_provider.dart index fe11ed5a..3996a088 100644 --- a/lib/providers/download_history_provider.dart +++ b/lib/providers/download_history_provider.dart @@ -44,6 +44,16 @@ bool historyItemsReferToSameStoredFile( ).any((key) => firstKeys.contains(key)); } +String? resolvePersistedHistoryQuality({ + required String? incoming, + required String? existing, +}) { + return nonPlaceholderQuality(incoming) ?? + nonPlaceholderQuality(existing) ?? + normalizeOptionalString(incoming) ?? + normalizeOptionalString(existing); +} + class DownloadHistoryItem { final String id; final String trackName; @@ -1034,6 +1044,16 @@ class DownloadHistoryNotifier extends Notifier { final mergedItem = existing == null ? incomingItem : incomingItem.copyWith( + quality: resolvePersistedHistoryQuality( + incoming: item.quality, + existing: existing.quality, + ), + bitDepth: item.bitDepth ?? existing.bitDepth, + sampleRate: item.sampleRate ?? existing.sampleRate, + bitrate: item.bitrate ?? existing.bitrate, + format: + normalizeOptionalString(item.format) ?? + normalizeOptionalString(existing.format), trackNumber: item.trackNumber ?? existing.trackNumber, totalTracks: item.totalTracks ?? existing.totalTracks, discNumber: item.discNumber ?? existing.discNumber, @@ -1223,6 +1243,21 @@ class DownloadHistoryNotifier extends Notifier { return DownloadHistoryItem.fromJson(json); } + Future getByFilePathAsync(String filePath) async { + final targetKeys = buildPathMatchKeys(filePath); + if (targetKeys.isEmpty) return null; + + for (final item in state.lookupItems) { + if (buildPathMatchKeys(item.filePath).any(targetKeys.contains)) { + return item; + } + } + + final json = await _db.findByFilePath(filePath); + if (json == null) return null; + return DownloadHistoryItem.fromJson(json); + } + Future findByTrackAndArtistAsync( String trackName, String artistName, diff --git a/lib/providers/download_queue_provider.dart b/lib/providers/download_queue_provider.dart index 0c5b22e6..310cf8a7 100644 --- a/lib/providers/download_queue_provider.dart +++ b/lib/providers/download_queue_provider.dart @@ -51,6 +51,18 @@ const String safPermissionLostErrorMessage = const String downloadFolderAccessLostErrorMessage = 'Download folder access lost. Please re-select your download folder in Settings.'; +/// Keeps a download in its finalizing state until its durable Library record +/// has been written. If persistence fails, completion is deliberately not +/// published so the queue can surface the error instead of losing the file +/// from the app while it still exists on disk. +Future persistBeforePublishingDownloadCompletion({ + required Future Function() persist, + required void Function() publish, +}) async { + await persist(); + publish(); +} + final _invalidFolderChars = RegExp(r'[<>:"/\\|?*]'); final _trimDotsAndSpacesRegex = RegExp(r'^[. ]+|[. ]+$'); final _trimUnderscoresAndSpacesRegex = RegExp(r'^[_ ]+|[_ ]+$'); @@ -3705,12 +3717,11 @@ class DownloadQueueNotifier extends Notifier { } } - updateItemStatus( - item.id, - DownloadStatus.completed, - progress: 1.0, - filePath: filePath, - ); + if (normalizeOptionalString(filePath) == null) { + throw StateError( + 'Download backend reported success without a final file path', + ); + } if (effectiveSafMode && filePath != null && isContentUri(filePath)) { await _saveExternalLrc( @@ -3754,37 +3765,19 @@ class DownloadQueueNotifier extends Notifier { _log.w('Album ReplayGain check failed: $e'); } - _completedInSession++; - final historyNotifier = ref.read(downloadHistoryProvider.notifier); - final existingInHistory = - await historyNotifier.getBySpotifyIdAsync(trackToDownload.id) ?? - (trackToDownload.isrc != null - ? await historyNotifier.getByIsrcAsync(trackToDownload.isrc!) - : null); + final existingInHistory = filePath == null + ? null + : await historyNotifier.getByFilePathAsync(filePath); if (wasExisting && existingInHistory != null) { - _log.i('Track already in library, skipping history update'); - await _notificationService.showDownloadComplete( - trackName: item.track.name, - artistName: item.track.artistName, - completedCount: _completedInSession, - totalCount: _totalQueuedAtStart, - alreadyInLibrary: true, + _log.i( + 'Track file already exists in library; refreshing its history metadata', ); - removeItem(item.id); - return; } - await _notificationService.showDownloadComplete( - trackName: item.track.name, - artistName: item.track.artistName, - completedCount: _completedInSession, - totalCount: _totalQueuedAtStart, - alreadyInLibrary: wasExisting, - ); - if (filePath != null) { + final historyFilePath = filePath; final backendBitDepth = result['actual_bit_depth'] as int?; final backendSampleRate = result['actual_sample_rate'] as int?; final backendFormat = @@ -3893,32 +3886,50 @@ class DownloadQueueNotifier extends Notifier { final historySampleRate = isLossyOutput ? null : finalSampleRate; final historyBitrate = isLossyOutput ? finalBitrateKbps : null; - if (settings.saveDownloadHistory) { - await ref - .read(downloadHistoryProvider.notifier) - .addToHistory( - _historyItemFromResult( - item: item, - trackToDownload: trackToDownload, - result: result, - filePath: filePath, - quality: actualQuality, - useSaf: effectiveSafMode, - downloadTreeUri: settings.downloadTreeUri, - safRelativeDir: effectiveOutputDir, - safFileName: finalSafFileName ?? safFileName, - bitDepth: historyBitDepth, - sampleRate: historySampleRate, - bitrate: historyBitrate, - format: finalFormat, - genre: effectiveGenre, - label: effectiveLabel, - copyright: effectiveCopyright, - ), - preserveTrackVariant: item.preserveQualityVariant, - ); - } - + await persistBeforePublishingDownloadCompletion( + persist: () async { + if (!settings.saveDownloadHistory) return; + await ref + .read(downloadHistoryProvider.notifier) + .addToHistory( + _historyItemFromResult( + item: item, + trackToDownload: trackToDownload, + result: result, + filePath: historyFilePath, + quality: actualQuality, + useSaf: effectiveSafMode, + downloadTreeUri: settings.downloadTreeUri, + safRelativeDir: effectiveOutputDir, + safFileName: finalSafFileName ?? safFileName, + bitDepth: historyBitDepth, + sampleRate: historySampleRate, + bitrate: historyBitrate, + format: finalFormat, + genre: effectiveGenre, + label: effectiveLabel, + copyright: effectiveCopyright, + ), + preserveTrackVariant: item.preserveQualityVariant, + ); + }, + publish: () { + _completedInSession++; + updateItemStatus( + item.id, + DownloadStatus.completed, + progress: 1.0, + filePath: filePath, + ); + }, + ); + await _notificationService.showDownloadComplete( + trackName: item.track.name, + artistName: item.track.artistName, + completedCount: _completedInSession, + totalCount: _totalQueuedAtStart, + alreadyInLibrary: wasExisting, + ); removeItem(item.id); } } else { diff --git a/lib/providers/download_queue_provider_native_worker.dart b/lib/providers/download_queue_provider_native_worker.dart index f2b3bd8d..bc7cd924 100644 --- a/lib/providers/download_queue_provider_native_worker.dart +++ b/lib/providers/download_queue_provider_native_worker.dart @@ -926,48 +926,55 @@ extension _DownloadQueueNativeWorker on DownloadQueueNotifier { } if (result['native_finalized'] == true) { - updateItemStatus( - item.id, - DownloadStatus.completed, - progress: 1.0, - filePath: filePath, - ); - if (settings.saveDownloadHistory) { - final historyItem = result['history_item']; - if (historyItem is Map) { - try { + final nativeFinalizedFilePath = filePath; + await persistBeforePublishingDownloadCompletion( + persist: () async { + if (!settings.saveDownloadHistory) return; + final historyItem = result['history_item']; + if (historyItem is Map) { + try { + await ref + .read(downloadHistoryProvider.notifier) + .adoptNativeHistoryItem( + DownloadHistoryItem.fromJson( + Map.from(historyItem), + ), + preserveTrackVariant: item.preserveQualityVariant, + ); + } catch (e) { + _log.w('Failed to adopt native history item: $e'); + await _persistNativeFinalizedHistoryFallback( + context, + result, + nativeFinalizedFilePath, + ); + } + } else if (result['history_written'] == true || + result['already_exists'] == true) { await ref .read(downloadHistoryProvider.notifier) - .adoptNativeHistoryItem( - DownloadHistoryItem.fromJson( - Map.from(historyItem), - ), - preserveTrackVariant: item.preserveQualityVariant, - ); - } catch (e) { - _log.w('Failed to adopt native history item: $e'); + .reloadFromStorage(); + } else { + _log.w( + 'Native finalizer completed without history; persisting Dart fallback', + ); await _persistNativeFinalizedHistoryFallback( context, result, - filePath, + nativeFinalizedFilePath, ); } - } else if (result['history_written'] == true) { - await ref.read(downloadHistoryProvider.notifier).reloadFromStorage(); - } else if (result['already_exists'] == true) { - await ref.read(downloadHistoryProvider.notifier).reloadFromStorage(); - } else { - _log.w( - 'Native finalizer completed without history; persisting Dart fallback', + }, + publish: () { + _completedInSession++; + updateItemStatus( + item.id, + DownloadStatus.completed, + progress: 1.0, + filePath: nativeFinalizedFilePath, ); - await _persistNativeFinalizedHistoryFallback( - context, - result, - filePath, - ); - } - } - _completedInSession++; + }, + ); await _notificationService.showDownloadComplete( trackName: item.track.name, artistName: item.track.artistName, @@ -1130,12 +1137,6 @@ extension _DownloadQueueNativeWorker on DownloadQueueNotifier { if (finalQuality != null) actualQuality = finalQuality; } - updateItemStatus( - item.id, - DownloadStatus.completed, - progress: 1.0, - filePath: filePath, - ); await _saveExternalLrc( result: result, settings: settings, @@ -1160,15 +1161,6 @@ extension _DownloadQueueNativeWorker on DownloadQueueNotifier { onFetchError: (e) => _log.w('Failed to fetch native-worker external LRC: $e'), ); - _completedInSession++; - - await _notificationService.showDownloadComplete( - trackName: item.track.name, - artistName: item.track.artistName, - completedCount: _completedInSession, - totalCount: _totalQueuedAtStart, - alreadyInLibrary: result['already_exists'] == true, - ); final resultSafFileName = result['file_name'] as String?; final lowerFilePath = filePath.toLowerCase(); @@ -1186,36 +1178,56 @@ extension _DownloadQueueNativeWorker on DownloadQueueNotifier { lowerFilePath.endsWith('.opus') || lowerFilePath.endsWith('.ogg'); - if (settings.saveDownloadHistory) { - await ref - .read(downloadHistoryProvider.notifier) - .addToHistory( - _historyItemFromResult( - item: item, - trackToDownload: trackToDownload, - result: result, - filePath: filePath, - quality: actualQuality, - useSaf: context.storageMode == 'saf', - downloadTreeUri: context.downloadTreeUri, - safRelativeDir: context.safRelativeDir, - safFileName: - (resultSafFileName != null && resultSafFileName.isNotEmpty) - ? resultSafFileName - : context.safFileName, - bitDepth: isLossyOutput ? null : actualBitDepth, - sampleRate: isLossyOutput ? null : actualSampleRate, - bitrate: isLossyOutput ? actualBitrate : null, - format: historyFormat, - genre: normalizeOptionalString(result['genre'] as String?), - label: normalizeOptionalString(result['label'] as String?), - copyright: normalizeOptionalString( - result['copyright'] as String?, + final completedFilePath = filePath; + await persistBeforePublishingDownloadCompletion( + persist: () async { + if (!settings.saveDownloadHistory) return; + await ref + .read(downloadHistoryProvider.notifier) + .addToHistory( + _historyItemFromResult( + item: item, + trackToDownload: trackToDownload, + result: result, + filePath: completedFilePath, + quality: actualQuality, + useSaf: context.storageMode == 'saf', + downloadTreeUri: context.downloadTreeUri, + safRelativeDir: context.safRelativeDir, + safFileName: + (resultSafFileName != null && resultSafFileName.isNotEmpty) + ? resultSafFileName + : context.safFileName, + bitDepth: isLossyOutput ? null : actualBitDepth, + sampleRate: isLossyOutput ? null : actualSampleRate, + bitrate: isLossyOutput ? actualBitrate : null, + format: historyFormat, + genre: normalizeOptionalString(result['genre'] as String?), + label: normalizeOptionalString(result['label'] as String?), + copyright: normalizeOptionalString( + result['copyright'] as String?, + ), ), - ), - preserveTrackVariant: item.preserveQualityVariant, - ); - } + preserveTrackVariant: item.preserveQualityVariant, + ); + }, + publish: () { + _completedInSession++; + updateItemStatus( + item.id, + DownloadStatus.completed, + progress: 1.0, + filePath: completedFilePath, + ); + }, + ); + await _notificationService.showDownloadComplete( + trackName: item.track.name, + artistName: item.track.artistName, + completedCount: _completedInSession, + totalCount: _totalQueuedAtStart, + alreadyInLibrary: result['already_exists'] == true, + ); removeItem(item.id); } diff --git a/test/download_history_logic_test.dart b/test/download_history_logic_test.dart index 1f95b842..1100f6ec 100644 --- a/test/download_history_logic_test.dart +++ b/test/download_history_logic_test.dart @@ -1,5 +1,5 @@ import 'package:flutter_test/flutter_test.dart'; -import 'package:spotiflac_android/providers/download_history_provider.dart'; +import 'package:spotiflac_android/providers/download_queue_provider.dart'; DownloadHistoryItem _historyItem({ required String id, @@ -82,6 +82,44 @@ void main() { ); expect(state.items, hasLength(2)); }); + + test('actual audio quality survives history serialization', () { + final item = + _historyItem( + id: 'quality', + filePath: '/music/Album/Hi-Res Song.flac', + downloadedAt: DateTime.utc(2026, 7, 23), + ).copyWith( + quality: '24-bit/96kHz', + bitDepth: 24, + sampleRate: 96000, + format: 'flac', + ); + + final restored = DownloadHistoryItem.fromJson(item.toJson()); + + expect(restored.quality, '24-bit/96kHz'); + expect(restored.bitDepth, 24); + expect(restored.sampleRate, 96000); + expect(restored.format, 'flac'); + }); + + test('placeholder refresh cannot erase an existing measured quality', () { + expect( + resolvePersistedHistoryQuality( + incoming: 'HI_RES_LOSSLESS', + existing: '24-bit/96kHz', + ), + '24-bit/96kHz', + ); + expect( + resolvePersistedHistoryQuality( + incoming: '24-bit/192kHz', + existing: '24-bit/96kHz', + ), + '24-bit/192kHz', + ); + }); }); group('startup orphan reconciliation', () { @@ -130,4 +168,37 @@ void main() { expect(decision.pendingIds, isEmpty); }); }); + + group('download completion persistence', () { + test('publishes completion only after history persistence', () async { + final events = []; + + await persistBeforePublishingDownloadCompletion( + persist: () async { + events.add('persist'); + }, + publish: () => events.add('publish'), + ); + + expect(events, ['persist', 'publish']); + }); + + test( + 'does not publish completion when history persistence fails', + () async { + var published = false; + + await expectLater( + persistBeforePublishingDownloadCompletion( + persist: () => + Future.error(StateError('database unavailable')), + publish: () => published = true, + ), + throwsStateError, + ); + + expect(published, isFalse); + }, + ); + }); }