immich-app/immich
1
0
Fork 0
mirror of https://github.com/immich-app/immich.git synced 2026-07-16 05:34:30 +03:00
Code Issues 783 Packages Projects Releases 22 Wiki Activity

[PR #15127] [MERGED] fix(mobile): 14983 Images upload to shared album with common name #14419

New Issue
Closed
opened 2026-02-05 15:31:46 +03:00 by OVERLORD · 0 comments
OVERLORD commented 2026-02-05 15:31:46 +03:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/immich-app/immich/pull/15127
Author: @Tyris
Created: 1/7/2025
Status: ✅ Merged
Merged: 1/17/2025
Merged by: @alextran1502

Base: main ← Head: fix/14983-shared-albums-with-common-name-causes-upload-to-wrong-album

📝 Commits (10+)

  • 81c1a4f Initial look at fixing issue where images are uploaded to the wrong album if a shared album conflicts with a local users album.
  • e35465c Merge branch 'main' into fix/14983-shared-albums-with-common-name-causes-upload-to-wrong-album
  • a42911b Use owner instead of shared flag when fetching albums.
  • 979ce90 Fix issue with refreshRemoteAlbums getting shared items twice and removed incorrect isShared comment.
  • 3a92fa8 Fix formatting.
  • c38f5af Fixed tests.
  • cbf0588 Merge branch 'main' into fix/14983-shared-albums-with-common-name-causes-upload-to-wrong-album
  • 7ce9448 Merge branch 'main' into fix/14983-shared-albums-with-common-name-causes-upload-to-wrong-album
  • b190385 Revert "Fixed tests."
  • d971d39 Revert "Fix issue with refreshRemoteAlbums getting shared items twice and removed incorrect isShared comment."

📊 Changes

4 files changed (+48 additions, -8 deletions)

View changed files

📝 mobile/lib/interfaces/album.interface.dart (+1 -0)
📝 mobile/lib/providers/album/album.provider.dart (+13 -3)
📝 mobile/lib/repositories/album.repository.dart (+15 -1)
📝 mobile/lib/services/album.service.dart (+19 -4)

📄 Description

This PR should address #14983.

The issue is caused because the client just asks for the first album with the matching name - which can result in the shared folder being returned.

The methods in question should never be uploading to an album owned by someone else, so setting shared to false in these cases should work (assuming I understand "shared" correctly - ie: that the item is shared with me).

Above assumption was wrong - have used ownerId instead.

Testing performed:

  • Old code (reproduce issue)
    • Create a new user with an album called "Test" and share that to my original user
    • Use the original user to upload an album called "Test" using the "Sync albums" option
    • Verify that files end up in the wrong album ✅
  • Using fixed code (verify fix)
    • Using the new user, create an album called "Test2" and share that to my original user
    • Use the original user to upload an album called "Test2" using the "Sync albums" option
    • Verify that a new album was created and images are not synced to the wrong album ✅
  • Using fixed code (verify edge case)
    • Using the original user, create an album called "Test3"
    • Use the original user to upload an album called "Test3" using the "Sync albums" option
    • Verify that the images go to the correct existing album ✅

Other notes:

  • Messing around with creating/deleting folders and adding to them can cause pretty weird issues because the "album exists" check is done locally; so if the albums aren't synced it will result in creation of a new album with that same name. This was apparent when creating new albums remotely and then immediately doing a sync for an album with the same name on the client - a new album would be created causing a duplicate (yuk!)

    In general, the sync system seems to be a little unreliable across these types of cases... Not sure the best long-term solution - a unified storage engine that handles remote and local through a single interface (think Firebase RTDB) would probably be the cleanest... I tried forcing an remote album refresh before adding assets, but this was non-trivial and expensive.

  • Related to my investigation of the above, the refreshRemoteAlbums() call has some strange logic that "works" but isn't really what was intended when written.

    Using getAll(shared: true) gets all shared albums the user can access (regardless of owner, despite the previous comment stating "owned if isShared is false)).

    Using getAll(shared: null) gets all albums (incuding shared = true and shared = false).

    I presume the intent here was to get albums that were shared to me (but not owned by me), and not shared (ie: mine), but the logic is way off since (because shared is set to null, instead of false; but also because shared doesn't actually do this!). It also just then combines them - so makes more sense to just get them in a single call.

    Have simplified this code to do what it should do (without the confusing bits) - would appreciate feedback on this commit in particular: 979ce9

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/immich-app/immich/pull/15127 **Author:** [@Tyris](https://github.com/Tyris) **Created:** 1/7/2025 **Status:** ✅ Merged **Merged:** 1/17/2025 **Merged by:** [@alextran1502](https://github.com/alextran1502) **Base:** `main` ← **Head:** `fix/14983-shared-albums-with-common-name-causes-upload-to-wrong-album` --- ### 📝 Commits (10+) - [`81c1a4f`](https://github.com/immich-app/immich/commit/81c1a4f1da2e893eb617631fc59d756e4122c771) Initial look at fixing issue where images are uploaded to the wrong album if a shared album conflicts with a local users album. - [`e35465c`](https://github.com/immich-app/immich/commit/e35465ccf308f52a46cbf54bffcc4be41fc5cb52) Merge branch 'main' into fix/14983-shared-albums-with-common-name-causes-upload-to-wrong-album - [`a42911b`](https://github.com/immich-app/immich/commit/a42911b2901ecc74aed1581a96c56c85af4313d6) Use owner instead of shared flag when fetching albums. - [`979ce90`](https://github.com/immich-app/immich/commit/979ce90abf22c6cd80ff06f61a08d0c03092991b) Fix issue with refreshRemoteAlbums getting shared items twice and removed incorrect isShared comment. - [`3a92fa8`](https://github.com/immich-app/immich/commit/3a92fa896e30233848e8518cee005c21a49967f8) Fix formatting. - [`c38f5af`](https://github.com/immich-app/immich/commit/c38f5af5acfc38444b00ed716755c241d297216a) Fixed tests. - [`cbf0588`](https://github.com/immich-app/immich/commit/cbf0588a6dd0f2c0decabc264fe59867a1f527c1) Merge branch 'main' into fix/14983-shared-albums-with-common-name-causes-upload-to-wrong-album - [`7ce9448`](https://github.com/immich-app/immich/commit/7ce9448a27753da637d831fc2486c3c224be2324) Merge branch 'main' into fix/14983-shared-albums-with-common-name-causes-upload-to-wrong-album - [`b190385`](https://github.com/immich-app/immich/commit/b190385b72f529e1b0664dd33057f874e0a18521) Revert "Fixed tests." - [`d971d39`](https://github.com/immich-app/immich/commit/d971d39c67ea221c6479debd6d5ec1cb700b39fa) Revert "Fix issue with refreshRemoteAlbums getting shared items twice and removed incorrect isShared comment." ### 📊 Changes **4 files changed** (+48 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `mobile/lib/interfaces/album.interface.dart` (+1 -0) 📝 `mobile/lib/providers/album/album.provider.dart` (+13 -3) 📝 `mobile/lib/repositories/album.repository.dart` (+15 -1) 📝 `mobile/lib/services/album.service.dart` (+19 -4) </details> ### 📄 Description This PR should address #14983. The issue is caused because the client just asks for the first album with the matching name - which can result in the shared folder being returned. ~~The methods in question should never be uploading to an album owned by someone else, so setting shared to false in these cases should work (assuming I understand "shared" correctly - ie: that the item is shared with me).~~ Above assumption was wrong - have used ownerId instead. Testing performed: - Old code (reproduce issue) - Create a new user with an album called "Test" and share that to my original user - Use the original user to upload an album called "Test" using the "Sync albums" option - Verify that files end up in the wrong album ✅ - Using fixed code (verify fix) - Using the new user, create an album called "Test2" and share that to my original user - Use the original user to upload an album called "Test2" using the "Sync albums" option - Verify that a new album was created and images are not synced to the wrong album ✅ - Using fixed code (verify edge case) - Using the original user, create an album called "Test3" - Use the original user to upload an album called "Test3" using the "Sync albums" option - Verify that the images go to the correct existing album ✅ Other notes: - Messing around with creating/deleting folders and adding to them can cause pretty weird issues because the "album exists" check is done locally; so if the albums aren't synced it will result in creation of a new album with that same name. This was apparent when creating new albums remotely and then immediately doing a sync for an album with the same name on the client - a new album would be created causing a duplicate (yuk!) In general, the sync system seems to be a little unreliable across these types of cases... Not sure the best long-term solution - a unified storage engine that handles remote and local through a single interface (think Firebase RTDB) would probably be the cleanest... I tried forcing an remote album refresh before adding assets, but this was non-trivial and expensive. - Related to my investigation of the above, the `refreshRemoteAlbums()` call has some strange logic that "works" but isn't really what was intended when written. > Using `getAll(shared: true)` gets all shared albums the user can access (regardless of owner, despite the previous comment stating "owned if `isShared` is false)). > > Using `getAll(shared: null)` gets all albums (incuding shared = true and shared = false). > > I presume the intent here was to get albums that were shared to me (but not owned by me), and not shared (ie: mine), but the logic is way off since (because shared is set to null, instead of false; but also because shared doesn't actually do this!). It also just then combines them - so makes more sense to just get them in a single call. Have simplified this code to do what it should do (without the confusing bits) - would appreciate feedback on this commit in particular: [979ce9](https://github.com/immich-app/immich/pull/15127/commits/979ce90abf22c6cd80ff06f61a08d0c03092991b) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
OVERLORD added the pull-request label 2026-02-05 15:31:46 +03:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
accessibility
changelog:enhancement
changelog:security
changelog:skip
changelog:translation
cli
date-time
dependencies
documentation
external-library
format
good first issue
mobile-beta
mobile-beta
mobile-beta
needs-answer
nice to have
pull-request
Mirrored from GitHub Pull Request
 sharing
tech-debt
📱mobile
🖥️web
🗄️server
🧠machine-learning
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
OVERLORD
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: immich-app/immich#14419
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?