fix: Populate photo neighbors and album context when deep linking#4273
fix: Populate photo neighbors and album context when deep linking#4273diusazzad wants to merge 2 commits intoLycheeOrg:masterfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new API endpoint for fetching individual photos with neighbor (previous/next) information. It adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0afd2ac-1f00-464a-acfb-a7c6da5b49a6
📒 Files selected for processing (8)
app/Actions/Photo/GetNeighbors.phpapp/Http/Controllers/Gallery/PhotoController.phpapp/Http/Requests/Photo/GetPhotoRequest.phpapp/Http/Resources/Models/PhotoResource.phpdatabase/migrations/2019_12_25_0600_config_exiftool_ternary.phpresources/js/stores/PhotoState.tsresources/js/views/gallery-panels/Album.vueroutes/api_v2.php
| public function do(Photo $photo, AbstractAlbum $album): array | ||
| { | ||
| $sorting = $album->getEffectivePhotoSorting(); | ||
| $column = $sorting->column->toColumn(); |
There was a problem hiding this comment.
Handle raw-ordering sort columns (e.g., rating_avg) explicitly.
The query uses plain column comparisons/orderBy for all sorts. That bypasses ColumnSortingType::requiresRawOrdering() behavior and can produce invalid/incorrect neighbor selection for raw-expression sorts.
Also applies to: 53-53, 67-73, 78-79
| { | ||
| $sorting = $album->getEffectivePhotoSorting(); | ||
| $column = $sorting->column->toColumn(); | ||
| $order = $sorting->order->toOrder(); |
There was a problem hiding this comment.
Normalize enum order before direction checks.
Line 64 assumes lowercase ('asc'). If sort order is uppercase enum value ('ASC'/'DESC'), neighbor direction logic is wrong for ascending sorts.
Suggested fix
- $order = $sorting->order->toOrder();
+ $order = strtolower($sorting->order->value);
...
- $is_asc = ($order === 'asc');
+ $is_asc = ($order === 'asc');Also applies to: 64-66
| $album = $photo->albums()->first(); | ||
| $neighbors = ['previous_photo_id' => null, 'next_photo_id' => null]; | ||
|
|
||
| if ($album !== null) { | ||
| $neighbors = $get_neighbors->do($photo, $album); | ||
| } | ||
|
|
||
| return new PhotoResource( | ||
| photo: $photo, | ||
| album_id: $album?->id, | ||
| should_downgrade_size_variants: !Gate::check(PhotoPolicy::CAN_ACCESS_FULL_PHOTO, [Photo::class, $photo]), | ||
| next_photo_id: $neighbors['next_photo_id'], |
There was a problem hiding this comment.
Use explicit album context instead of first() for multi-album photos.
Line 76 picks an arbitrary attached album. For photos in multiple albums, this can return wrong album_id and wrong neighbors for the viewer’s current route context.
Please resolve album context from request (e.g., optional album_id) and validate membership/access before calling GetNeighbors.
| if (this.photo === undefined) { | ||
| const response = await PhotoService.get(this.photoId); | ||
| this.photo = response.data; | ||
| } |
There was a problem hiding this comment.
Guard against stale async responses overwriting the current photo.
On Line 55, PhotoService.get(...) can resolve after photoId has changed; Line 56 then assigns outdated data into this.photo.
Suggested fix
if (this.photo === undefined) {
- const response = await PhotoService.get(this.photoId);
- this.photo = response.data;
+ const requested_photo_id = this.photoId;
+ const response = await PhotoService.get(requested_photo_id);
+ if (this.photoId !== requested_photo_id) {
+ return;
+ }
+ this.photo = response.data;
}📝 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.photo === undefined) { | |
| const response = await PhotoService.get(this.photoId); | |
| this.photo = response.data; | |
| } | |
| if (this.photo === undefined) { | |
| const requested_photo_id = this.photoId; | |
| const response = await PhotoService.get(requested_photo_id); | |
| if (this.photoId !== requested_photo_id) { | |
| return; | |
| } | |
| this.photo = response.data; | |
| } |
| orderManagement.load(); | ||
| photoStore.photoId = photoId.value; | ||
| photoStore.load(); | ||
| await photoStore.load(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace await usage in this Vue component with .then() chaining.
This introduces await in Vue code paths (Lines 228, 243, 498, 518), which conflicts with the project Vue convention.
As per coding guidelines, **/*.vue: “Do not use await async calls in Vue3, use .then() instead”.
Also applies to: 243-243, 498-498, 518-518
|
I already replied on #4272 that this was not the way to do it. You completely ignored my advises and implementation directions. Closing. |
2 participants
This Pull Request addresses the issue where deep-linking directly to a photo (e.g., gallery/<photo_id>) results in a lack of navigation context (Next/Previous photo) and a null album ID on the frontend.
Previously, the backend's PhotoController@get method hardcoded album_id as null and did not provide information about surrounding photos. This forced the frontend to manually manage state, which failed for non-initial page photos and direct entry points.
Changes:
Backend Calculation of Neighbors: Introduced a new action app/Actions/Photo/GetNeighbors.php that calculates the previous and next photo IDs within a given album's context, respecting its specific sorting settings (column and order).
PhotoResource Update: Modified the PhotoResource constructor to accept and return next_photo_id and previous_photo_id.
Controller Logic: Updated PhotoController@get to identify a suitable album for the photo (if it belongs to any) and use the GetNeighbors action to populate the navigation context.
This ensures that direct links have fully functional navigation buttons from the moment they are loaded.
Related Issues
Fixes #4260 (indirectly, as part of the discussion in #4272).
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes