diff --git a/mobile/lib/infrastructure/repositories/timeline.repository.dart b/mobile/lib/infrastructure/repositories/timeline.repository.dart index 9c4501b665..d9cd347dec 100644 --- a/mobile/lib/infrastructure/repositories/timeline.repository.dart +++ b/mobile/lib/infrastructure/repositories/timeline.repository.dart @@ -181,7 +181,8 @@ class DriftTimelineRepository extends DriftDatabaseRepository { TimelineQuery remoteAlbum(String albumId, GroupAssetsBy groupBy) => ( bucketSource: () => _watchRemoteAlbumBucket(albumId, groupBy: groupBy), - assetSource: (offset, count) => _getRemoteAlbumBucketAssets(albumId, offset: offset, count: count), + assetSource: (offset, count) => + _getRemoteAlbumBucketAssets(albumId, groupBy: groupBy, offset: offset, count: count), origin: TimelineOrigin.remoteAlbum, ); @@ -235,7 +236,12 @@ class DriftTimelineRepository extends DriftDatabaseRepository { .handleError((error) => const []); } - Future> _getRemoteAlbumBucketAssets(String albumId, {required int offset, required int count}) async { + Future> _getRemoteAlbumBucketAssets( + String albumId, { + required int offset, + required int count, + GroupAssetsBy groupBy = GroupAssetsBy.day, + }) async { final albumData = await (_db.remoteAlbumEntity.select()..where((row) => row.id.equals(albumId))).getSingleOrNull(); // If album doesn't exist (was deleted), return empty list @@ -262,11 +268,14 @@ class DriftTimelineRepository extends DriftDatabaseRepository { ), ])..where(_db.remoteAssetEntity.deletedAt.isNull() & _db.remoteAlbumAssetEntity.albumId.equals(albumId)); - if (isAscending) { - query.orderBy([OrderingTerm.asc(_db.remoteAssetEntity.createdAt)]); - } else { - query.orderBy([OrderingTerm.desc(_db.remoteAssetEntity.createdAt)]); - } + // Order assets by the same effective date the buckets group by, otherwise offset + // paging puts an asset whose localDateTime differs from createdAt under the wrong + // date header (#28852). createdAt is the within-day tiebreak. + OrderingTerm ord(Expression exp) => isAscending ? OrderingTerm.asc(exp) : OrderingTerm.desc(exp); + query.orderBy([ + if (groupBy != GroupAssetsBy.none) ord(_db.remoteAssetEntity.effectiveCreatedAt(groupBy)), + ord(_db.remoteAssetEntity.createdAt), + ]); query.limit(count, offset: offset); @@ -373,13 +382,14 @@ class DriftTimelineRepository extends DriftDatabaseRepository { TimelineQuery place(String place, GroupAssetsBy groupBy) => ( bucketSource: () => _watchPlaceBucket(place, groupBy: groupBy), - assetSource: (offset, count) => _getPlaceBucketAssets(place, offset: offset, count: count), + assetSource: (offset, count) => _getPlaceBucketAssets(place, groupBy: groupBy, offset: offset, count: count), origin: TimelineOrigin.place, ); TimelineQuery person(String userId, String personId, GroupAssetsBy groupBy) => ( bucketSource: () => _watchPersonBucket(userId, personId, groupBy: groupBy), - assetSource: (offset, count) => _getPersonBucketAssets(userId, personId, offset: offset, count: count), + assetSource: (offset, count) => + _getPersonBucketAssets(userId, personId, groupBy: groupBy, offset: offset, count: count), origin: TimelineOrigin.person, ); @@ -416,7 +426,12 @@ class DriftTimelineRepository extends DriftDatabaseRepository { }).watch(); } - Future> _getPlaceBucketAssets(String place, {required int offset, required int count}) { + Future> _getPlaceBucketAssets( + String place, { + required int offset, + required int count, + GroupAssetsBy groupBy = GroupAssetsBy.day, + }) { final query = _db.remoteAssetEntity.select().join([ innerJoin( @@ -430,7 +445,11 @@ class DriftTimelineRepository extends DriftDatabaseRepository { _db.remoteAssetEntity.visibility.equalsValue(AssetVisibility.timeline) & _db.remoteExifEntity.city.equals(place), ) - ..orderBy([OrderingTerm.desc(_db.remoteAssetEntity.createdAt)]) + // Match the bucket grouping (#28852); place buckets are always date-grouped. + ..orderBy([ + OrderingTerm.desc(_db.remoteAssetEntity.effectiveCreatedAt(groupBy)), + OrderingTerm.desc(_db.remoteAssetEntity.createdAt), + ]) ..limit(count, offset: offset); return query.map((row) => row.readTable(_db.remoteAssetEntity).toDto()).get(); } @@ -486,6 +505,7 @@ class DriftTimelineRepository extends DriftDatabaseRepository { String personId, { required int offset, required int count, + GroupAssetsBy groupBy = GroupAssetsBy.day, }) { final idQuery = _db.assetFaceEntity.selectOnly() ..addColumns([_db.assetFaceEntity.assetId]) @@ -503,7 +523,11 @@ class DriftTimelineRepository extends DriftDatabaseRepository { row.ownerId.equals(userId) & row.visibility.equalsValue(AssetVisibility.timeline), ) - ..orderBy([(row) => OrderingTerm.desc(row.createdAt)]) + // Match the bucket grouping (#28852); createdAt is the within-day tiebreak. + ..orderBy([ + if (groupBy != GroupAssetsBy.none) (row) => OrderingTerm.desc(row.effectiveCreatedAt(groupBy)), + (row) => OrderingTerm.desc(row.createdAt), + ]) ..limit(count, offset: offset); return query.map((row) => row.toDto()).get(); diff --git a/mobile/test/medium/repositories/timeline_repository_test.dart b/mobile/test/medium/repositories/timeline_repository_test.dart index f604f53c3e..a2d0e7aefa 100644 --- a/mobile/test/medium/repositories/timeline_repository_test.dart +++ b/mobile/test/medium/repositories/timeline_repository_test.dart @@ -46,6 +46,40 @@ void main() { expect((assets.first as RemoteAsset).id, remoteAsset.id); expect([localAsset1.id, localAsset2.id], contains((assets.first as RemoteAsset).localId)); }); + + test('orders assets by effective date so they land under the correct date bucket (#28852)', () async { + // Buckets group by the effective date = coalesce(localDateTime, createdAt). The asset + // list must use the same ordering, otherwise offset paging puts an asset whose + // localDateTime differs from createdAt under the wrong date header. + final user = await ctx.newUser(); + final album = await ctx.newRemoteAlbum(ownerId: user.id, order: .desc); + // A: shown on Sep 3 (localDateTime) but only has the earlier Sep 2 createdAt. + final assetA = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 2, 12), + localDateTime: DateTime.utc(2024, 9, 3, 12), + ); + // B: the inverse — shown on Sep 2 but has the later Sep 3 createdAt. + final assetB = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 3, 12), + localDateTime: DateTime.utc(2024, 9, 2, 12), + ); + await ctx.newRemoteAlbumAsset(albumId: album.id, assetId: assetA.id); + await ctx.newRemoteAlbumAsset(albumId: album.id, assetId: assetB.id); + + final query = sut.remoteAlbum(album.id, .day); + + final buckets = await query.bucketSource().first; + expect(buckets, hasLength(2)); + expect(buckets.every((b) => b.assetCount == 1), isTrue); + + // Buckets are ordered by effective date desc (Sep 3 then Sep 2), so the asset list + // must be A then B. The pre-fix raw-createdAt ordering returned B first (Sep 3 createdAt), + // which slotted B under the Sep 3 header and A under Sep 2. + final assets = await query.assetSource(0, 10); + expect(assets.map((a) => (a as RemoteAsset).id).toList(), [assetA.id, assetB.id]); + }); }); group('person assets', () { @@ -69,5 +103,33 @@ void main() { expect(assets, hasLength(1)); expect((assets.first as RemoteAsset).id, asset.id); }); + + test('orders assets by effective date so they land under the correct date bucket (#28852)', () async { + final user = await ctx.newUser(); + final person = await ctx.newPerson(ownerId: user.id); + // A shown on Sep 3 (localDateTime) with the earlier Sep 2 createdAt; B is the inverse. + final assetA = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 2, 12), + localDateTime: DateTime.utc(2024, 9, 3, 12), + ); + final assetB = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 3, 12), + localDateTime: DateTime.utc(2024, 9, 2, 12), + ); + await ctx.newFace(assetId: assetA.id, personId: person.id); + await ctx.newFace(assetId: assetB.id, personId: person.id); + + final query = sut.person(user.id, person.id, .day); + + final buckets = await query.bucketSource().first; + expect(buckets, hasLength(2)); + + // Buckets are date-desc (Sep 3 then Sep 2); the asset list must match (A then B). + // The pre-fix raw-createdAt order returned B first. + final assets = await query.assetSource(0, 10); + expect(assets.map((a) => (a as RemoteAsset).id).toList(), [assetA.id, assetB.id]); + }); }); } diff --git a/mobile/test/medium/repository_context.dart b/mobile/test/medium/repository_context.dart index ed06774e82..6f1b5694b4 100644 --- a/mobile/test/medium/repository_context.dart +++ b/mobile/test/medium/repository_context.dart @@ -102,6 +102,7 @@ class MediumRepositoryContext { String? stackId, String? thumbHash, String? libraryId, + DateTime? localDateTime, }) async { id ??= TestUtils.uuid(); createdAt ??= TestUtils.date(); @@ -125,7 +126,7 @@ class MediumRepositoryContext { isEdited: .new(isEdited ?? false), livePhotoVideoId: .new(livePhotoVideoId), stackId: .new(stackId), - localDateTime: .new(createdAt.toLocal()), + localDateTime: .new(localDateTime ?? createdAt.toLocal()), thumbHash: .new(TestUtils.uuid(thumbHash)), libraryId: .new(TestUtils.uuid(libraryId)), ),