Add albums to tag albums and person albums - #4626
Conversation
📝 WalkthroughWalkthroughChangesThe pull request adds cached, paginated real-album listings for Tag Albums and Person Albums. It centralizes person matching, adds configuration and translations, loads matching albums in the client, batches photo events, and invalidates related caches after photo changes. Matching album listings
Album listing cache invalidation
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to Album listings can remain stale after root moves, tag criteria changes, or maintenance failures, causing users to see removed or outdated matching albums. The PR should not merge until these cache-invalidation paths are corrected or explicitly accepted by the owner. Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
app/Services/PersonAlbumMatcher.php (1)
114-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the by-reference parameter and the unused sub-select list.
Builderis an object. PHP passes the handle by value, so&$queryhas no effect and the docblock@param Builder &$queryis misleading. In thewhereExistssub-query the selected columns are ignored by the database;COUNT(DISTINCT person_id) AS numis never read.♻️ Proposed refactor
- private function getPhotoIdsWithPersons(Builder &$query, array $person_ids, bool $is_and): void + private function getPhotoIdsWithPersons(Builder $query, array $person_ids, bool $is_and): void { @@ $query->whereExists( - fn (BaseBuilder $q) => $q->select(['photo_id', DB::raw('COUNT(DISTINCT person_id) AS num')]) + fn (BaseBuilder $q) => $q->selectRaw('1') ->from('faces')app/Repositories/AlbumRepository.php (2)
159-188: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConfirm the AND path scales with the number of criteria tags.
Each required tag adds one correlated
whereHas('tags', ...)subquery. For a TagAlbum with many criteria tags this produces one EXISTS per tag on every page load. An alternative is a singlewhereHas('tags', fn ($q) => $q->whereIn('tags.id', $tag_ids), '=', count($tag_ids))which uses one subquery with a count constraint. Verify which form your target databases plan better before changing it.
244-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the
whereExistsclosure parameter.
fn ($q) => ...has no type hint, unlike the other closures in this file. Add\Illuminate\Database\Query\Builder $qso PHPStan level 6 keeps the inference and the code matches the surrounding style.tests/Unit/Repositories/AlbumRepositoryTest.php (1)
356-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a cache-hit test for the person variant.
The tag variant has
testGetMatchingAlbumsForTagPaginatedCacheHitPerformsNoAlbumsTableQueries. The person variant has no equivalent. The person path builds a different cache key and a different tag set, so it can regress independently.💚 Proposed test
public function testGetMatchingAlbumsForPersonPaginatedCacheHitPerformsNoAlbumsTableQueries(): void { $person = Person::factory()->create(['is_searchable' => true]); $photo = Photo::factory()->owned_by($this->user)->in($this->parentAlbum)->create(); Face::factory()->for_photo($photo)->for_person($person)->create(); $this->actingAs($this->user); $personAlbum = resolve(CreatePersonAlbum::class)->create('person_album', [$person->id], false); $this->repository->getMatchingAlbumsForPersonPaginated($personAlbum, 10); $second_call_count = $this->countAlbumTableQueries( fn () => $this->repository->getMatchingAlbumsForPersonPaginated($personAlbum, 10) ); $this->assertSame(0, $second_call_count, 'A cache hit must not run any query against albums/base_albums.'); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f94644ef-4cd2-4517-8b50-eaefec178fb9
📒 Files selected for processing (41)
app/Actions/Album/Merge.phpapp/Actions/Photo/MoveOrDuplicate.phpapp/DTO/Delete/PhotosToBeDeletedDTO.phpapp/Events/PhotoPersonsChanged.phpapp/Http/Controllers/AiVision/FaceController.phpapp/Http/Controllers/Gallery/AlbumChildrenController.phpapp/Http/Requests/Album/GetAlbumChildrenRequest.phpapp/Http/Resources/Collections/PaginatedAlbumsResource.phpapp/Listeners/ManagedCacheAlbumListingInvalidator.phpapp/Providers/EventServiceProvider.phpapp/Relations/HasManyPhotosByPerson.phpapp/Repositories/AlbumRepository.phpapp/Services/Cache/CacheKeyProvider.phpapp/Services/PersonAlbumMatcher.phpdatabase/migrations/2026_08_16_000005_add_matching_albums_listing_config.phplang/ar/all_settings.phplang/bg/all_settings.phplang/cz/all_settings.phplang/de/all_settings.phplang/el/all_settings.phplang/en/all_settings.phplang/es/all_settings.phplang/fa/all_settings.phplang/fr/all_settings.phplang/hu/all_settings.phplang/it/all_settings.phplang/ja/all_settings.phplang/nl/all_settings.phplang/no/all_settings.phplang/pl/all_settings.phplang/pt/all_settings.phplang/ru/all_settings.phplang/sk/all_settings.phplang/sv/all_settings.phplang/tr/all_settings.phplang/vi/all_settings.phplang/zh_CN/all_settings.phplang/zh_TW/all_settings.phpresources/js/stores/AlbumState.tstests/Feature_v2/Album/AlbumMatchingAlbumsTest.phptests/Unit/Repositories/AlbumRepositoryTest.php
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review.
| // Dispatch PhotoSaved for every photo that gained this album link | ||
| // (covers both cross-album move and the same-album "copy" case, | ||
| // i.e. CopyPhotosRequest, where $from_album === $to_album and | ||
| // PhotoMoved below never fires) so listeners depending on a | ||
| // photo's containing albums (e.g. the TagAlbum/PersonAlbum | ||
| // "matching albums" cache) are notified either way. | ||
| foreach ($photos_ids as $photo_id) { | ||
| PhotoSaved::dispatch($photo_id); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Invalidate person-album caches when a photo moves to the root.
If $from_album !== null and $to_album === null, the code removes the source pivot row but does not dispatch PhotoSaved. It also does not dispatch PhotoMoved. PhotoDeleted has no managed-cache invalidator registration in app/Providers/EventServiceProvider.php.
Cached matching albums can retain the source album for persons on the moved photo. Dispatch an invalidating event after any album-link mutation, including a move to the root. Add a regression test for this path.
Proposed fix
if ($to_album !== null) {
// Add the new links.
DB::table(PA::PHOTO_ALBUM)->insert(array_map(fn (string $id) => ['photo_id' => $id, 'album_id' => $to_album->id], $photos_ids));
// Dispatch event for destination album (photos added)
AlbumSaved::dispatch([$to_album->id], [$to_album->parent_id]);
-
- foreach ($photos_ids as $photo_id) {
- PhotoSaved::dispatch($photo_id);
- }
}
+ if ($from_album !== null || $to_album !== null) {
+ foreach ($photos_ids as $photo_id) {
+ PhotoSaved::dispatch($photo_id);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Dispatch PhotoSaved for every photo that gained this album link | |
| // (covers both cross-album move and the same-album "copy" case, | |
| // i.e. CopyPhotosRequest, where $from_album === $to_album and | |
| // PhotoMoved below never fires) so listeners depending on a | |
| // photo's containing albums (e.g. the TagAlbum/PersonAlbum | |
| // "matching albums" cache) are notified either way. | |
| foreach ($photos_ids as $photo_id) { | |
| PhotoSaved::dispatch($photo_id); | |
| } | |
| if ($to_album !== null) { | |
| // Add the new links. | |
| DB::table(PA::PHOTO_ALBUM)->insert(array_map(fn (string $id) => ['photo_id' => $id, 'album_id' => $to_album->id], $photos_ids)); | |
| // Dispatch event for destination album (photos added) | |
| AlbumSaved::dispatch([$to_album->id], [$to_album->parent_id]); | |
| } | |
| if ($from_album !== null || $to_album !== null) { | |
| // Dispatch PhotoSaved for every photo whose album links changed. | |
| foreach ($photos_ids as $photo_id) { | |
| PhotoSaved::dispatch($photo_id); | |
| } | |
| } |
| $tags = [ | ||
| ...$this->cache_key_provider->albumTagTags($tag_ids), | ||
| $this->cache_key_provider->userTag($user_id), | ||
| $this->cache_key_provider->albumListingGlobalTag(), | ||
| ]; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Matching-albums cache entries omit the owning album's cache tag. Both new cached listings are tagged with criteria tags, the user tag, and the global listing tag, but not with albumTag() of the TagAlbum/PersonAlbum itself. ManagedCacheAlbumListingInvalidator::handleTagAlbumSaved() and handlePersonAlbumSaved() evict exactly that missing tag, so a criteria edit on the smart album leaves the stale page in the cache.
app/Repositories/AlbumRepository.php#L133-L137: add$this->cache_key_provider->albumTag($tag_album->id)to the$tagsarray.app/Repositories/AlbumRepository.php#L219-L223: add$this->cache_key_provider->albumTag($person_album->id)to the$tagsarray.
📍 Affects 1 file
app/Repositories/AlbumRepository.php#L133-L137(this comment)app/Repositories/AlbumRepository.php#L219-L223
| if ($this->config_manager->getValueAsBool('PA_override_visibility')) { | ||
| $this->photo_query_policy | ||
| ->applySensitivityFilter( | ||
| query: $ids_query, | ||
| user: $user, | ||
| origin: null, | ||
| include_nsfw: !$this->config_manager->getValueAsBool('hide_nsfw_in_person_albums') | ||
| ) | ||
| ->where(fn (Builder $q) => $this->getPhotoIdsWithPersons($q, $person_ids, $album->is_and)); | ||
| } else { | ||
| $this->photo_query_policy | ||
| ->applySearchabilityFilter( | ||
| query: $ids_query, | ||
| user: $user, | ||
| unlocked_album_ids: $unlocked_album_ids, | ||
| origin: null, | ||
| include_nsfw: !$this->config_manager->getValueAsBool('hide_nsfw_in_person_albums') | ||
| ) | ||
| ->where(fn (Builder $q) => $this->getPhotoIdsWithPersons($q, $person_ids, $album->is_and)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the duplicated predicate from both branches.
Both branches append the identical ->where(fn (Builder $q) => $this->getPhotoIdsWithPersons(...)) clause. The coding guidelines forbid duplicate code in both the if and else statements. Apply the person predicate once after the visibility filter.
As per coding guidelines: "Avoid code duplication in both if and else statements".
♻️ Proposed refactor
if ($this->config_manager->getValueAsBool('PA_override_visibility')) {
$this->photo_query_policy
->applySensitivityFilter(
query: $ids_query,
user: $user,
origin: null,
include_nsfw: !$this->config_manager->getValueAsBool('hide_nsfw_in_person_albums')
- )
- ->where(fn (Builder $q) => $this->getPhotoIdsWithPersons($q, $person_ids, $album->is_and));
+ );
} else {
$this->photo_query_policy
->applySearchabilityFilter(
query: $ids_query,
user: $user,
unlocked_album_ids: $unlocked_album_ids,
origin: null,
include_nsfw: !$this->config_manager->getValueAsBool('hide_nsfw_in_person_albums')
- )
- ->where(fn (Builder $q) => $this->getPhotoIdsWithPersons($q, $person_ids, $album->is_and));
+ );
}
+
+ $ids_query->where(fn (Builder $q) => $this->getPhotoIdsWithPersons($q, $person_ids, $album->is_and));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($this->config_manager->getValueAsBool('PA_override_visibility')) { | |
| $this->photo_query_policy | |
| ->applySensitivityFilter( | |
| query: $ids_query, | |
| user: $user, | |
| origin: null, | |
| include_nsfw: !$this->config_manager->getValueAsBool('hide_nsfw_in_person_albums') | |
| ) | |
| ->where(fn (Builder $q) => $this->getPhotoIdsWithPersons($q, $person_ids, $album->is_and)); | |
| } else { | |
| $this->photo_query_policy | |
| ->applySearchabilityFilter( | |
| query: $ids_query, | |
| user: $user, | |
| unlocked_album_ids: $unlocked_album_ids, | |
| origin: null, | |
| include_nsfw: !$this->config_manager->getValueAsBool('hide_nsfw_in_person_albums') | |
| ) | |
| ->where(fn (Builder $q) => $this->getPhotoIdsWithPersons($q, $person_ids, $album->is_and)); | |
| } | |
| if ($this->config_manager->getValueAsBool('PA_override_visibility')) { | |
| $this->photo_query_policy | |
| ->applySensitivityFilter( | |
| query: $ids_query, | |
| user: $user, | |
| origin: null, | |
| include_nsfw: !$this->config_manager->getValueAsBool('hide_nsfw_in_person_albums') | |
| ); | |
| } else { | |
| $this->photo_query_policy | |
| ->applySearchabilityFilter( | |
| query: $ids_query, | |
| user: $user, | |
| unlocked_album_ids: $unlocked_album_ids, | |
| origin: null, | |
| include_nsfw: !$this->config_manager->getValueAsBool('hide_nsfw_in_person_albums') | |
| ); | |
| } | |
| $ids_query->where(fn (Builder $q) => $this->getPhotoIdsWithPersons($q, $person_ids, $album->is_and)); |
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Listeners/WebhookListener.php (1)
57-62: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBatch move-event handling and regression coverage should be tightened.
handlePhotoMoved()currently reloads enabled webhooks and photo relations once per photo ID, creating repeated queries for large batches. Reuse the batch-level webhook collection and batch-load photos before constructing per-photo payloads. Add multi-ID regression coverage for overlapping memberships, empty batches, inactive webhooks, and one dispatch per photo ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fc0f06c4-baad-483c-88e0-603cc5c30200
📒 Files selected for processing (16)
app/Actions/Album/Merge.phpapp/Actions/Photo/MoveOrDuplicate.phpapp/Actions/Photo/Pipes/Shared/SetParent.phpapp/Events/PhotoMoved.phpapp/Events/PhotoSaved.phpapp/Http/Controllers/Admin/Maintenance/GenSizeVariants.phpapp/Http/Controllers/Admin/Maintenance/MissingFileSizes.phpapp/Listeners/ManagedCacheAlbumListingInvalidator.phpapp/Listeners/RecomputeAlbumSizeOnPhotoMutation.phpapp/Listeners/RecomputeAlbumStatsOnPhotoChange.phpapp/Listeners/RecomputeAlbumUserThumbsOnPhotoChange.phpapp/Listeners/WebhookListener.phptests/Precomputing/CoverSelection/EventListenersTest.phptests/Precomputing/CoverSelection/EventPropagationIntegrationTest.phptests/Precomputing/SizeComputations/AlbumSizeEventListenerTest.phptests/Unit/Listeners/WebhookListenerTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Actions/Album/Merge.php
- app/Listeners/ManagedCacheAlbumListingInvalidator.php
Included review availability: Your plan includes up to 8 reviews per rolling hour; 6 remain after this review.
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests