Fix deep link failure for non-initial page photos (#4260)#4272
Fix deep link failure for non-initial page photos (#4260)#4272diusazzad wants to merge 1 commit intoLycheeOrg:masterfrom
Conversation
📝 WalkthroughWalkthroughThe PR adds a complete single-photo fetch endpoint to the API, including a new controller method, request validation with authorization checks, a corresponding route, and updates the frontend to support asynchronous photo loading with fallback remote fetching. 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 |
|
Hi, thank you for the tentative, but this is not the way to solve this issue. Yes you get the photo, however you do not have the next-previous behaviour. A better approach would be to query the page the photo is in and load that page (before loading the previous pages of that album). Once you have the data of the album in, you can fetch from the cache. Yes it is more complex, but it aligns with the current behavior of the front-end rather than making it an exception for a single photo. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
database/migrations/2019_12_25_0600_config_exiftool_ternary.php (1)
27-36:⚠️ Potential issue | 🟡 MinorRemove the unreachable
$path === nullcheck on line 32.Since
Safe\execreturns astringtype and throws exceptions on failure rather than returningnull, the condition$path === nullwill never be true. It should be removed:Proposed fix
- if ($path === '' || $path === null) { + if ($path === '') {Additionally, note that
ConfigManager::hasExiftool()(line 191) uses plainexec()and only has Unix-specific detection (command -v exiftool), while this migration now adds Windows support. This creates a potential inconsistency on Windows systems when the config value is reset to2at runtime—the migration would correctly detect exiftool, but the runtime check would fail. Consider updatingConfigManager::hasExiftool()to use the same OS-aware detection logic.resources/js/stores/PhotoState.ts (1)
40-57:⚠️ Potential issue | 🟠 MajorStabilize async photo loading against stale responses and unhandled fetch errors.
load()now performs network I/O but lacks protection against two hazards. First,this.photoIdcan change between theawaitat line 55 and the state assignment at line 56, causing an older request's response to overwrite a newer navigation state. Second,PhotoService.get()can throw (HTTP 404/403/network failures), but there is no error handler, so exceptions will bubble to callers inresources/js/views/gallery-panels/Timeline.vue(line 479),resources/js/views/gallery-panels/Search.vue(lines 218, 255, 530), andresources/js/views/gallery-panels/Tag.vue(line 303), which do not await this call. Snapshot the requested id before awaiting, guard all state mutations against stale responses, and normalize failures to keep the store API safe.🛠️ Suggested hardening
- async load() { - if (this.photoId === undefined) { + async load() { + const requestedPhotoId = this.photoId; + if (requestedPhotoId === undefined) { this.photo = undefined; return; } - if (this.photo?.id === this.photoId) { + if (this.photo?.id === requestedPhotoId) { // Already loaded return; } const photosState = usePhotosStore(); - this.photo = photosState.photos.find((p) => p.id === this.photoId); - - if (this.photo === undefined) { - const response = await PhotoService.get(this.photoId); - this.photo = response.data; - } + const cachedPhoto = photosState.photos.find((p) => p.id === requestedPhotoId); + if (cachedPhoto !== undefined) { + if (this.photoId === requestedPhotoId) { + this.photo = cachedPhoto; + } + return; + } + + try { + const response = await PhotoService.get(requestedPhotoId); + if (this.photoId === requestedPhotoId) { + this.photo = response.data; + } + } catch { + if (this.photoId === requestedPhotoId) { + this.photo = undefined; + } + } },
🧹 Nitpick comments (1)
resources/js/views/gallery-panels/Album.vue (1)
228-228: Prefer.then()for the new photo-load flow.These additions introduce new
await/asyncusage inside a.vuefile. The logic is fine, but it drifts from the repo’s Vue convention and makes this view inconsistent with the rest of the frontend async flow.As per coding guidelines,
**/*.vue: Do not use await async calls in Vue3, use .then() instead.Also applies to: 243-243, 498-519
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3265333a-722f-41ff-93d6-848daf16c2e2
📒 Files selected for processing (6)
app/Http/Controllers/Gallery/PhotoController.phpapp/Http/Requests/Photo/GetPhotoRequest.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 get(GetPhotoRequest $request): PhotoResource | ||
| { | ||
| return new PhotoResource( | ||
| photo: $request->photo(), | ||
| album_id: null, | ||
| should_downgrade_size_variants: !Gate::check(PhotoPolicy::CAN_ACCESS_FULL_PHOTO, [Photo::class, $request->photo()]) | ||
| ); |
There was a problem hiding this comment.
Return album-scoped photo data here.
This endpoint hard-codes album_id: null, so the PhotoResource built for an off-page photo loses its album context. That leaves next_photo_id and previous_photo_id unset, which breaks next/previous navigation, keyboard navigation, and slideshow continuity for photos opened through this fallback path. Please carry the current album id through this request and validate it before building the resource.
|
You open #4273 which is pretty much this with a patch to make the left right work. This is NOT the solution we want. We want something that is properly structured. I gave you directions in which to focus on, you could have for example added a query string which reference the current photo page in the album. This could be updated when scrolling down for example, but no you ignored and persisted. Closing. |
2 participants
This PR resolves an issue where direct links (deep links) to photos would fail if the target photo was not part of the initial paginated load of the album. This happened because the frontend relied solely on the local photosStore cache and lacked a mechanism to fetch individual photo details from the backend when missing.
Summary by CodeRabbit
New Features
Chores