From 5a457d72c97ee25704f5af460ca9d91f90b91e0c Mon Sep 17 00:00:00 2001 From: Peter Ombodi Date: Mon, 27 Apr 2026 16:16:49 +0300 Subject: [PATCH] fix(mobile): delete assets on trash empty, Android (#26070) * fix(mobile): improve trash sync flow - trash local assets on remote delete events - unify remote trash handling and support assetDelete cleanup by remote asset id - update sync stream tests * fix(mobile): revert pubspec.lock * refactor(mobile): remove helper remove unused columns from results * refactor(mobile): use remoteIds in getAssetsFromBackupAlbums and remove getAssetsFromBackupAlbumsByRemoteIds refactor tests --------- Co-authored-by: Peter Ombodi --- .../domain/services/sync_stream.service.dart | 71 +++++++++++++------ .../repositories/local_asset.repository.dart | 23 ++++-- .../services/sync_stream_service_test.dart | 14 ++-- 3 files changed, 75 insertions(+), 33 deletions(-) diff --git a/mobile/lib/domain/services/sync_stream.service.dart b/mobile/lib/domain/services/sync_stream.service.dart index b98ba24407..01772683f7 100644 --- a/mobile/lib/domain/services/sync_stream.service.dart +++ b/mobile/lib/domain/services/sync_stream.service.dart @@ -3,6 +3,7 @@ import 'dart:async'; import 'dart:convert'; +import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/store.model.dart'; import 'package:immich_mobile/domain/models/sync_event.model.dart'; import 'package:immich_mobile/entities/store.entity.dart'; @@ -191,17 +192,22 @@ class SyncStreamService { case SyncEntityType.assetV1: final remoteSyncAssets = data.cast(); await _syncStreamRepository.updateAssetsV1(remoteSyncAssets); - if (CurrentPlatform.isAndroid && Store.get(StoreKey.manageLocalMediaAndroid, false)) { - final hasPermission = await _localFilesManager.hasManageMediaPermission(); - if (hasPermission) { - await _handleRemoteTrashed(remoteSyncAssets.where((e) => e.deletedAt != null).map((e) => e.checksum)); + await _runWithManageMediaPermission( + logContext: "Trashed Assets", + action: () async { + await _handleRemoteDeleted(remoteSyncAssets.where((e) => e.deletedAt != null).map((e) => e.id)); await _applyRemoteRestoreToLocal(); - } else { - _logger.warning("sync Trashed Assets cannot proceed because MANAGE_MEDIA permission is missing"); - } - } + }, + ); return; case SyncEntityType.assetDeleteV1: + await _runWithManageMediaPermission( + logContext: "Deleted Assets", + action: () async { + final remoteSyncAssets = data.cast(); + await _handleRemoteDeleted(remoteSyncAssets.map((e) => e.assetId)); + }, + ); return _syncStreamRepository.deleteAssetsV1(data.cast()); case SyncEntityType.assetExifV1: return _syncStreamRepository.updateAssetsExifV1(data.cast()); @@ -382,28 +388,32 @@ class SyncStreamService { } } - Future _handleRemoteTrashed(Iterable checksums) async { - if (checksums.isEmpty) { + Future _handleRemoteDeleted(Iterable remoteIds) async { + if (remoteIds.isEmpty) { return Future.value(); } else { - final localAssetsToTrash = await _localAssetRepository.getAssetsFromBackupAlbums(checksums); + final localAssetsToTrash = await _localAssetRepository.getAssetsFromBackupAlbums(remoteIds); if (localAssetsToTrash.isNotEmpty) { - final mediaUrls = await Future.wait( - localAssetsToTrash.values - .expand((e) => e) - .map((localAsset) => _storageRepository.getAssetEntityForAsset(localAsset).then((e) => e?.getMediaUrl())), - ); - _logger.info("Moving to trash ${mediaUrls.join(", ")} assets"); - final result = await _localFilesManager.moveToTrash(mediaUrls.nonNulls.toList()); - if (result) { - await _trashedLocalAssetRepository.trashLocalAsset(localAssetsToTrash); - } + await _trashLocalAssets(localAssetsToTrash); } else { - _logger.info("No assets found in backup-enabled albums for assets: $checksums"); + _logger.info("No assets found in backup-enabled albums for remote assets: $remoteIds"); } } } + Future _trashLocalAssets(Map> localAssetsToTrash) async { + final mediaUrls = await Future.wait( + localAssetsToTrash.values + .expand((e) => e) + .map((localAsset) => _storageRepository.getAssetEntityForAsset(localAsset).then((e) => e?.getMediaUrl())), + ); + _logger.info("Moving to trash ${mediaUrls.join(", ")} assets"); + final result = await _localFilesManager.moveToTrash(mediaUrls.nonNulls.toList()); + if (result) { + await _trashedLocalAssetRepository.trashLocalAsset(localAssetsToTrash); + } + } + Future _applyRemoteRestoreToLocal() async { final assetsToRestore = await _trashedLocalAssetRepository.getToRestore(); if (assetsToRestore.isNotEmpty) { @@ -413,4 +423,21 @@ class SyncStreamService { _logger.info("No remote assets found for restoration"); } } + + Future _runWithManageMediaPermission({ + required String logContext, + required Future Function() action, + }) async { + if (!CurrentPlatform.isAndroid || !Store.get(StoreKey.manageLocalMediaAndroid, false)) { + return; + } + + final hasPermission = await _localFilesManager.hasManageMediaPermission(); + if (!hasPermission) { + _logger.warning("sync $logContext cannot proceed because MANAGE_MEDIA permission is missing"); + return; + } + + await action(); + } } diff --git a/mobile/lib/infrastructure/repositories/local_asset.repository.dart b/mobile/lib/infrastructure/repositories/local_asset.repository.dart index 6f6ef20aeb..c34d2c4697 100644 --- a/mobile/lib/infrastructure/repositories/local_asset.repository.dart +++ b/mobile/lib/infrastructure/repositories/local_asset.repository.dart @@ -109,31 +109,40 @@ class DriftLocalAssetRepository extends DriftDatabaseRepository { return query.map((localAlbum) => localAlbum.toDto()).get(); } - Future>> getAssetsFromBackupAlbums(Iterable checksums) async { - if (checksums.isEmpty) { + Future>> getAssetsFromBackupAlbums(Iterable remoteIds) async { + if (remoteIds.isEmpty) { return {}; } final result = >{}; - for (final slice in checksums.toSet().slices(kDriftMaxChunk)) { + for (final slice in remoteIds.toSet().slices(kDriftMaxChunk)) { final rows = await (_db.select(_db.localAlbumAssetEntity).join([ - innerJoin(_db.localAlbumEntity, _db.localAlbumAssetEntity.albumId.equalsExp(_db.localAlbumEntity.id)), + innerJoin( + _db.localAlbumEntity, + _db.localAlbumAssetEntity.albumId.equalsExp(_db.localAlbumEntity.id), + useColumns: false, + ), innerJoin(_db.localAssetEntity, _db.localAlbumAssetEntity.assetId.equalsExp(_db.localAssetEntity.id)), + innerJoin( + _db.remoteAssetEntity, + _db.localAssetEntity.checksum.equalsExp(_db.remoteAssetEntity.checksum), + useColumns: false, + ), ])..where( _db.localAlbumEntity.backupSelection.equalsValue(BackupSelection.selected) & - _db.localAssetEntity.checksum.isIn(slice), + _db.remoteAssetEntity.id.isIn(slice), )) .get(); for (final row in rows) { final albumId = row.readTable(_db.localAlbumAssetEntity).albumId; - final assetData = row.readTable(_db.localAssetEntity); - final asset = assetData.toDto(); + final asset = row.readTable(_db.localAssetEntity).toDto(); (result[albumId] ??= []).add(asset); } } + return result; } diff --git a/mobile/test/domain/services/sync_stream_service_test.dart b/mobile/test/domain/services/sync_stream_service_test.dart index a182c6cdca..1bee1dccde 100644 --- a/mobile/test/domain/services/sync_stream_service_test.dart +++ b/mobile/test/domain/services/sync_stream_service_test.dart @@ -419,8 +419,8 @@ void main() { 'album-b': [mergedAsset], }; when(() => mockLocalAssetRepo.getAssetsFromBackupAlbums(any())).thenAnswer((invocation) async { - final Iterable requestedChecksums = invocation.positionalArguments.first as Iterable; - expect(requestedChecksums.toSet(), equals({'checksum-local', 'checksum-merged', 'checksum-remote-only'})); + final Iterable requestedRemoteIds = invocation.positionalArguments.first as Iterable; + expect(requestedRemoteIds.toSet(), equals({'remote-1', 'remote-2', 'remote-3'})); return assetsByAlbum; }); @@ -482,12 +482,18 @@ void main() { verifyNever(() => mockTrashedLocalAssetRepo.trashLocalAsset(any())); }); - test("does not request local deletions for permanent remote delete events", () async { + test("requests local deletions lookup by remote ids for permanent remote delete events", () async { + when(() => mockLocalAssetRepo.getAssetsFromBackupAlbums(any())).thenAnswer((invocation) async { + final Iterable requestedRemoteIds = invocation.positionalArguments.first as Iterable; + expect(requestedRemoteIds.toSet(), equals({'remote-asset'})); + return {}; + }); + final events = [SyncStreamStub.assetDeleteV1]; await simulateEvents(events); - verifyNever(() => mockLocalAssetRepo.getAssetsFromBackupAlbums(any())); + verify(() => mockLocalAssetRepo.getAssetsFromBackupAlbums(any())).called(1); verifyNever(() => mockLocalFilesManagerRepo.moveToTrash(any())); verify(() => mockSyncStreamRepo.deleteAssetsV1(any())).called(1); });