refactor: reenable cloud ids by shenlong-tanwen · Pull Request #30345 · immich-app/immich

shenlong-tanwen · 2026-07-28T19:32:54Z

Verified that the cloud ids are pushed to the server on upload & during the sync cloud id flow. Also verified that on a reinstall with no albums selected for backups, the checksums are reconciled based off of the cloud ids

immich-push-o-matic · 2026-07-28T19:40:35Z

📱 Android release APK (universal) — 010230b5f3d777554b255b58d2917bd683c37d5d

Download: https://github.com/immich-app/immich/actions/runs/31668778688/artifacts/9169073821

QR code

Installs as a separate app (applicationId app.alextran.immich.pr30345), so it coexists with the Play Store version and any other PR builds.

agg23

Why is this diff so large to push some IDs in a payload of an existing API call?

agg23 · 2026-08-11T19:41:44Z

    guard #available(iOS 16, *) else {
-      return assetIds.map { CloudIdResult(assetId: $0) }
+      return assetIds.map {
+        CloudIdResult(assetId: $0, error: "Cloud identifiers require iOS 16", errorKind: .unsupported)


This error return seems weird to me

It should be a bit more cleaner now as we do an early throw and handle it on the dart side rather than passing it back as an error kind

agg23 · 2026-08-11T19:42:49Z

+        let kind = cloudIdErrorKind(for: error)
+        var message = "Error getting Cloud Id: \(error.localizedDescription)"
+        if kind == .ambiguous,
+          let matches = (error as NSError).userInfo[PHLocalIdentifiersErrorKey] as? [String] {


If you're going to do this anyway, it seems like it should be part of the enum somehow

The enum is auto generated by pigeon and it cannot be a dart enhanced enum (the one with constructors / methods in it). I've moved the error mapping to the helper method now

agg23 · 2026-08-11T19:45:31Z

+import 'package:immich_mobile/platform/native_sync_api.g.dart';
+import 'package:logging/logging.dart';
+
+const kCloudIdChunkSize = 5000;


How is this number selected?

agg23 · 2026-08-11T19:47:28Z

+}) async {
+  final logger = Logger('resolveCloudIds');
+
+  for (int offset = 0; offset < assetIds.length; offset += kCloudIdChunkSize) {


nit: I imagine Dart has a nice functional way to chunk content, and if not, you could just do a fancy loop over ceil(assetIds.length/kCloudIdChunkSize)

I see this and, while C for loops are obviously incredibly common, I immediately think I have to carefully check your logic

Welp! Updated it to use slices now

agg23 · 2026-08-11T19:51:05Z

+      }
+
+      if (result.errorKind == CloudIdErrorKind.unsupported) {
+        logger.warning('Cloud IDs unavailable: ${result.error ?? "unsupported"}');


We always want to abort in this scenario?

Yes because if the platform does not support cloud ids, none of the others would resolve them as well. But the way this was handled previously was rather ugly with the looping. The new code now throws an error and we match on the specific case which is better than what we had previously

I did consider adding the iOS version check in the dart side before making the native call, but that adds a native call for all iOS versions but the failure is only on iOS 15 which is rather a small portion of devices, so decided to go with the exception approach for now

agg23 · 2026-08-11T19:52:04Z

+
+    final cloudMapping = <String, String>{};
+    for (final result in await nativeSyncApi.getCloudIdForAssetIds(chunk)) {
+      if (result.cloudId != null) {


I would do this logic the other way. Do your early returns with logging, then at the bottom have your actual work. The continue being used in your base case is the tell

I would also try to do it functionally, but that's OK if you don't want to

Good idea, updated it as such!

shenlong-tanwen · 2026-08-13T06:20:46Z

Why is this diff so large to push some IDs in a payload of an existing API call?

In theory, it should've just been few lines getting uncommented as we already had code for syncing the cloud ids to the database. But the reason this got a bit bigger is that the change also has a bit of refactor as to how we fetch said ids as we've disabled it previously because of performance issues we've faced with it then. Part of it was the DB hang issue we had frequently but that has been fixed since. Still, Apple calls out the call to fetch the ids to be expensive so I thought it'd be best if we did it in batches rather than in a single call. The API batching and DB batching are also part of the same optimisation so as to not send a bulk initial payload to the server

The limits I've selected are rather arbitrary. I landed on 5000 so the checkpoints (Native calls / DB update) runs frequently. If you'd rather want us to increase this limit, I can run some benchmarks around the time it takes for the native calls and we can increase it accordingly.

agg23 · 2026-08-13T13:28:10Z

  mocktail: ^1.0.5
  # Type safe platform code
-  pigeon: ^26.3.4
+  pigeon: ^27.3.0


Unrelated change?

agg23 · 2026-08-13T13:35:11Z

    dartPackageName: 'immich_mobile',
  ),
 )
+const String kUnSupportedOSError = 'UNSUPPORTED_OS';


nit: kUnsupportedOSError

agg23 · 2026-08-13T13:42:07Z

-
-    // await _localAlbumRepository.updateCloudMapping(cloudMapping);
+  Future<void> _mapCloudIds(List<LocalAsset> assets) async {
+    if (!CurrentPlatform.isIOS || assets.isEmpty) {


Now that I look at it again, this should not be gating resolveCloudIds and in fact all of the call sites should directly call resolveCloudIds.

resolveCloudIds can do the local optimization of only allowing iOS and !isEmpty. Otherwise your optimization is premature, and other callers can call resolveCloudIds in non-optimal scenarios

agg23 · 2026-08-13T13:42:27Z

+}) async {
+  final logger = Logger('resolveCloudIds');
+
+  for (final batch in assetIds.slices(kCloudIdChunkSize)) {


Thank you. I like this way more

agg23 · 2026-08-13T13:45:07Z

+        db.localAssetEntity,
+        db.localAssetEntity.id.isInQuery(
+          db.localAssetEntity.selectOnly()
+            ..addColumns([db.localAssetEntity.id.min()])


Is there a reason behind id.min() other than selecting a single row?

agg23 · 2026-08-13T14:01:25Z

+    ])
+    ..where(
+      db.remoteAssetEntity.ownerId.equals(userId) &
+          // Skip locked assets as we cannot update them without unlocking first


What does this mean? We're sending mappings to the server and it can't receive them because the assets are locked?

agg23 · 2026-08-13T14:02:19Z

+          db.localAssetEntity.iCloudId.isNotNull() &
+          // Only select assets that have a local cloud ID but either no remote cloud ID or a mismatched eTag
+          (db.remoteAssetCloudIdEntity.cloudId.isNull() |
+              db.remoteAssetCloudIdEntity.adjustmentTime.isNotExp(db.localAssetEntity.adjustmentTime) |


We should not be manually comparing these fields. I imagine this is done in a number of other places too. We need to have a single standard way of comparing

agg23 · 2026-08-13T14:03:13Z

+});
+
+@visibleForTesting
+Future<List<CloudIdMapping>> fetchMapping(Drift db, String userId, int limit, String? lastRemoteId) async {


I think this is of appropriate complexity that adding docstrings would be good. Particularly for what lastRemoteId is

agg23 · 2026-08-13T14:04:00Z

+    ..limit(limit);
+
+  if (lastRemoteId != null) {
+    query.where(db.remoteAssetEntity.id.isBiggerThanValue(lastRemoteId));


As per the docstring comment, idk what this means but it looks weird

Also, who uses "bigger than" instead of "greater than"?

agg23 · 2026-08-13T14:04:59Z

+});
+
+@visibleForTesting
+Future<List<CloudIdMapping>> fetchMapping(Drift db, String userId, int limit, String? lastRemoteId) async {


I see now this method was moved. While the ordering might be nice, ideally we don't want to have large diffs like this where we can't tell if it's new or not

shenlong-tanwen added 📱mobile changelog:skip labels Jul 28, 2026

shenlong-tanwen force-pushed the refactor/re-enable-cloud-ids branch from 770eab3 to bbafecf Compare July 31, 2026 12:35

shenlong-tanwen marked this pull request as ready for review July 31, 2026 12:35

shenlong-tanwen requested review from agg23 and santoshakil as code owners July 31, 2026 12:35

agg23 reviewed Aug 11, 2026

View reviewed changes

shenlong-tanwen added 3 commits August 13, 2026 01:00

refactor: reenable cloud ids

db60295

cleanup error formatting

ffdd360

throw on unsupported

de28149

shenlong-tanwen force-pushed the refactor/re-enable-cloud-ids branch from bbafecf to de28149 Compare August 13, 2026 02:43

shenlong-tanwen added 2 commits August 13, 2026 08:26

move to freezed and slice http batching

df060ee

simplify remote asset duplication handling

010230b

agg23 reviewed Aug 13, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

shenlong-tanwen commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

immich-push-o-matic Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agg23 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shenlong-tanwen commented Aug 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shenlong-tanwen commented Jul 28, 2026 •

edited

Loading

immich-push-o-matic Bot commented Jul 28, 2026 •

edited

Loading