Show album cover art while password-protected - #4704
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds manual album cover selection, validates cover ownership, and persists ChangesAlbum cover selection and visibility
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to Album updates may accept a cover from another album, while cover and sorting changes can leave gallery state stale or inconsistent. These paths should be corrected before merge. 🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Docstring CoverageExplanation Docstring coverage is 61.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 91 files. (5 skipped: 5 unsupported.) 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. A rabbit checks the cover, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Feature_v2/Album/AlbumsTest.php (1)
229-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse snake_case for the locked album variable.
Rename
$lockedAlbumto$locked_albumin both tests. Update all references in each test.
tests/Feature_v2/Album/AlbumsTest.php#L229-L250: Rename$lockedAlbumand its references to$locked_album.tests/Feature_v2/Album/AlbumsTest.php#L254-L276: Rename$lockedAlbumand its references to$locked_album.As per coding guidelines: “Variable names should be in snake_case.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 8d7ad47d-68b4-448e-afe3-12c2b33ae587
📒 Files selected for processing (47)
app/Actions/Admin/BulkEditAlbumsAction.phpapp/Actions/Album/SetProtectionPolicy.phpapp/Constants/AccessPermissionConstants.phpapp/Contracts/Http/Requests/RequestAttribute.phpapp/Http/Requests/Album/SetAlbumProtectionPolicyRequest.phpapp/Http/Resources/Models/ThumbAlbumResource.phpapp/Http/Resources/Models/Utils/AlbumProtectionPolicy.phpapp/Models/AccessPermission.phpdatabase/factories/AccessPermissionFactory.phpdatabase/migrations/2026_08_31_000000_add_grants_cover_access_to_access_permissions_table.phplang/ar/dialogs.phplang/bg/dialogs.phplang/cz/dialogs.phplang/de/dialogs.phplang/el/dialogs.phplang/en/dialogs.phplang/es/dialogs.phplang/fa/dialogs.phplang/fr/dialogs.phplang/hu/dialogs.phplang/it/dialogs.phplang/ja/dialogs.phplang/nl/dialogs.phplang/no/dialogs.phplang/pl/dialogs.phplang/pt/dialogs.phplang/ru/dialogs.phplang/sk/dialogs.phplang/sv/dialogs.phplang/tr/dialogs.phplang/vi/dialogs.phplang/zh_CN/dialogs.phplang/zh_TW/dialogs.phpresources/js/lychee.d.tsresources/js/services/album-service.tsresources/js/v7/components/forms/album/AlbumVisibility.vueresources/js/v7/components/gallery/albumModule/AlbumListItem.vueresources/js/v7/components/gallery/albumModule/thumbs/AlbumThumb.vueresources/js/v8/components/forms/album/AlbumVisibility.vueresources/js/v8/components/gallery/albumModule/AlbumListItem.vueresources/js/v8/components/gallery/albumModule/thumbs/AlbumThumb.vuetests/Feature_v2/Album/AlbumUpdateTest.phptests/Feature_v2/Album/AlbumsTest.phptests/Feature_v2/SmartAlbums/OverridePermissionsTest.phptests/Precomputing/CoverSelection/EventPropagationIntegrationTest.phptests/Unit/Actions/Album/SetProtectionPolicyTest.phptests/Unit/Http/Requests/Album/SetAlbumProtectionPolicyRequestTest.php
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
|
Hi @matthewbolding I am currently in the middle of a huge refactoring of the way albums are actually being displayed in the front-end. Do you mind we wait for it to be merged as it will have a LOT of changes with regard to your proposition? PS: I really like the idea BTW, it is just that it arrives at the worse moment and I don't want you to waste your time on something which is going to be removed soon. See here for what is coming: #4705 And yes, I am planning the same change for the photo display... |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
Hi @ildyria. Thanks for the reply. Yes, I have no issue waiting on other changes to the codebase to be applied before this feature gets incorporated. I'll likely reach out to you on Discord late on. Glad you like the idea! Another idea I had was to permit fully custom album covers, i.e., the system shall accept a new image file to be used for the sole purpose of an album cover. |
Isn't cover_id already enough for that ? |
Not necessarily. To my understanding, cover_id can only be a photo that already exists in the album (or sub-album, I suppose), but the cover_id cannot be, for instance, some standalone photo that's not meant to be part of the album. In my own deployments of Lychee, I've inserted custom JS to force the covers of certain top-level password-protected albums to be 1) visible to anonymous users and 2) be a custom image that's not part of the gallery. |
ildyria
added
parked - will revisit later
|
I had a look, pretty damn, good! On one end, a global setting would make it easier and avoid the Access rights propagation. In that specific case, given the complexity added by the flexibility I would suggest to go for the global config instead. Other questions are:
|
I think that having the option global would likely be the best good idea, at least initially, although I could certainly see a user make a compelling case for the setting to be album-by-album.
Yes, that's what I was thinking; if the option is enabled for the album cover to show while locked, then display it. I'm not particularly sure if differentiating between a specifically set cover_id and the auto-selected thumbnail is necessary, but that's just me. Did I understand your question properly? And another thought crossed my mind: replacing the lock icon with the cover will give the user no indication that a particular album is locked. Perhaps overlaying a transparent, small lock icon that's positioned, say, in the bottom right might be prudent. |
|
I think then the following is the best approach: Two separate settings:
The reason why the selected cover is interesting is because it allows the granularity you have from the per-album setting and it gives control on which thumb is selected, while if you just have the per album setting, you are exposed to a bit of randomness (the thumb selected depends of the ordering inside the album). With regard to locked, you can probably just put a span 50% with |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/Feature_v2/Album/AlbumsTest.php (1)
261-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse snake_case test variables. The project coding convention applies to this test file. Rename
$lockedAlbumto$locked_albumand$selectedCoverto$selected_cover, including all references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: dfa9a6d8-3479-42aa-8639-bb7e2e112b9a
📒 Files selected for processing (68)
app/Contracts/Http/Requests/RequestAttribute.phpapp/Http/Controllers/Gallery/AlbumController.phpapp/Http/Requests/Album/UpdateAlbumRequest.phpapp/Http/Resources/Models/ThumbAlbumResource.phpdatabase/migrations/2026_09_06_000000_add_locked_album_cover_configs.phpdatabase/migrations/2026_09_06_000001_drop_grants_cover_access_from_access_permissions_table.phplang/ar/all_settings.phplang/ar/gallery.phplang/bg/all_settings.phplang/bg/gallery.phplang/cz/all_settings.phplang/cz/gallery.phplang/de/all_settings.phplang/de/gallery.phplang/el/all_settings.phplang/el/gallery.phplang/en/all_settings.phplang/en/gallery.phplang/es/all_settings.phplang/es/gallery.phplang/fa/all_settings.phplang/fa/gallery.phplang/fr/all_settings.phplang/fr/gallery.phplang/hu/all_settings.phplang/hu/gallery.phplang/it/all_settings.phplang/it/gallery.phplang/ja/all_settings.phplang/ja/gallery.phplang/nl/all_settings.phplang/nl/gallery.phplang/no/all_settings.phplang/no/gallery.phplang/pl/all_settings.phplang/pl/gallery.phplang/pt/all_settings.phplang/pt/gallery.phplang/ru/all_settings.phplang/ru/gallery.phplang/sk/all_settings.phplang/sk/gallery.phplang/sv/all_settings.phplang/sv/gallery.phplang/tr/all_settings.phplang/tr/gallery.phplang/vi/all_settings.phplang/vi/gallery.phplang/zh_CN/all_settings.phplang/zh_CN/gallery.phplang/zh_TW/all_settings.phplang/zh_TW/gallery.phpresources/js/composables/contextMenus/contextMenu.tsresources/js/lychee.d.tsresources/js/services/album-service.tsresources/js/v7/components/forms/album/AlbumProperties.vueresources/js/v7/components/gallery/albumModule/AlbumPanel.vueresources/js/v7/views/gallery-panels/Search.vueresources/js/v8/components/forms/album/AlbumProperties.vueresources/js/v8/components/gallery/albumModule/AlbumPanel.vueresources/js/v8/composables/contextMenus/contextMenu.tsresources/js/v8/views/gallery-panels/Search.vuetests/Feature_v2/Album/AlbumMatchingAlbumsTest.phptests/Feature_v2/Album/AlbumTitleSyncTest.phptests/Feature_v2/Album/AlbumUpdateTest.phptests/Feature_v2/Album/AlbumsTest.phptests/Feature_v2/TitleSplitIntegrityTest.phptests/Unit/Http/Requests/Album/UpdateAlbumRequestTest.php
💤 Files with no reviewable changes (1)
- resources/js/lychee.d.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/Http/Controllers/Gallery/AlbumController.php (2)
135-135: 🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | ⚡ Quick winIDOR (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External · Exploitability: Moderate
Enforce album membership before persisting
cover_id.
UpdateAlbumRequest::processValidatedValues()resolves the submitted ID withPhoto::query()->findOrFail($cover_id), but it does not constrain the photo to the album being updated. This allows an existing photo from another album to become the target album’s cover. Resolve the photo through the target album’sphotos()relation before assigningcover_id.
158-164: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDispatch
AlbumChildrenChangedafter the queued recomputation.When
QUEUE_CONNECTIONuses an asynchronous driver,RecomputeChildAlbumBucketsJobcan execute itsupsert()after the controller synchronously invalidates the cache. A request can then repopulate the children cache with oldbucket_idvalues. DispatchAlbumChildrenChangedfrom the job after a successfulupsert(), asRecomputeRootAlbumBucketsJobdoes, and add a regression test.resources/js/v7/views/gallery-panels/Search.vue (1)
366-366: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate
editable.cover_idonly afterPhotoService.setAsCover()succeeds.Both Search panels update
modelAlbum.cover_idandeditable.cover_idbefore theAlbum::coverrequest settles. A failed request leaves the local value changed. The album editor later sends this value ascover_id, so a save can persist a cover that the server rejected. Use the successful response to refresh or synchronize the authoritative album state, and leave local state unchanged on failure. Apply this correction in both panels.resources/js/v8/components/gallery/albumModule/AlbumPanel.vue (1)
364-364: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInvalidate parent and root listing state after setting a cover.
AlbumService.clearCache(albumStore.album.id)does not remove the parent keyalbum_albums_<parent-id>_page<page>or the rootalbumsentry. Both cover callbacks can therefore leave parent or root navigation showing the previous cover. Use a shared cover-invalidation helper that clears the changed album, clears its parent listing or root listing as applicable, and invalidatesuseAlbumListStore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: fcde9932-90d5-41b7-a96d-58190229b7dd
📒 Files selected for processing (11)
app/Contracts/Http/Requests/RequestAttribute.phpapp/Http/Controllers/Gallery/AlbumController.phpresources/js/lychee.d.tsresources/js/services/album-service.tsresources/js/v7/views/gallery-panels/Search.vueresources/js/v8/components/gallery/albumModule/AlbumListItem.vueresources/js/v8/components/gallery/albumModule/AlbumPanel.vueresources/js/v8/components/gallery/albumModule/thumbs/AlbumThumb.vueresources/js/v8/utils/adaptAlbumChildTile.tsresources/js/v8/utils/adaptCategoryTile.tsresources/js/v8/views/gallery-panels/Search.vue
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
|
The two migrations which add and drop the |
ildyria
left a comment
ildyria
left a comment
There was a problem hiding this comment.
Just one fix on the validation that the photo belongs to albums (and descendants)
Adds a per-album 'grants_cover_access' toggle so a locked album can still display its cover thumbnail to anonymous visitors, without revealing photo contents. Defaults to off for every existing album.
It was never read by the front-end; is_locked already carries the state listing components actually branch on. Updated the two protection-policy tests to assert on is_locked/thumb instead.
Per maintainer feedback on the per-album grants_cover_access toggle (complexity of access-rights propagation, and friction with the upcoming Struct-of-Array album refactor), replace it with two global configs: show_cover_of_locked_albums (always show) and show_selected_cover_on_locked_albums (show only when the cover was manually chosen via "Set as Cover", not auto-selected). ThumbAlbumResource now decides locked-album thumb visibility from these configs instead of the per-album AccessPermission flag, which is dropped from the schema along with all its plumbing (model, factory, request validation, SetProtectionPolicy, BulkEditAlbumsAction, and the now-removed per-album toggle in both v7 and v8 AlbumVisibility forms). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVGbZsGYEK8nCGLfoXVmBN
"Set Album Cover" always showed even when right-clicking the photo (or sub-album) that was already the cover, so the only way to clear it was to click what looked like a no-op action. Mirror the existing set/remove pattern already used for the album header: show "Remove Album Cover" when the target is the current cover_id, "Set Album Cover" otherwise. Applied to both the shared (v7) and v8-specific contextMenu.ts composables. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVGbZsGYEK8nCGLfoXVmBN
Extend PATCH /Album (UpdateAlbumRequest/AlbumController::updateAlbum) to accept cover_id, mirroring the existing header_id handling exactly: validated as a nullable RandomID, membership-checked against the album's photos in authorize(), and persisted alongside the other fields in one save. EditableBaseAlbumResource already exposed cover_id for reading; this adds the write path. Add a "Set album cover" picker to the About Album tab in both v7 and v8, next to the existing header picker (v8 gates it behind Expert Mode like header already is). Also fix a real bug this exposed: the photo/sub-album context menu's "Set/Remove Album Cover" action only updated the top-level albumStore.modelAlbum/tagAlbum.cover_id, not the separate nested editable.cover_id copy that the About Album Select reads from, so a context-menu change never showed up there without a full reload. setAsHeader already had this sync for header_id; setAsCover was missing it entirely. Applied the same fix to both the photo-level and sub-album-level setAsCover callbacks, in both v7 and v8's AlbumPanel.vue and Search.vue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVGbZsGYEK8nCGLfoXVmBN
CodeRabbit flagged that photoMenu() checked selectors.album (the ref itself, always truthy) instead of selectors.album.value, so a photo refresh that resolves before albumStore.album is reassigned could crash the context menu when it dereferences cover_id/header_id/rights on undefined.
CodeRabbit found that when editable.cover_id points to a photo not in the currently loaded page of photosStore.photos, buildCoverId (v7 and v8) returned undefined for it. Since saveAlbum() maps a missing cover selection to cover_id: null, saving any unrelated field on the album silently cleared the real persisted cover. Return an id-only option instead so the id round-trips even when the photo record isn't loaded yet.
CodeRabbit noted the v8 cover USelectMenu had no way to remove an existing cover: Nuxt UI only renders its clear button when the clear prop is set. Add clear and widen cover_id's type to allow null, which Nuxt UI resets a cleared selection to; saveAlbum() already maps that to cover_id: null.
The multi-line condition and cast added in 45bacef didn't match Prettier's line-length rules; npm run check-formatting was failing CI.
The cover Select added in bc47266 exceeded Prettier's line length; this was already on origin and failing CI's formatting check.
Merging upstream/master brought in adaptAlbumChildTile.ts and adaptCategoryTile.ts (Feature 062's struct-of-arrays listing adapters), which predate is_locked on ThumbAlbumResource and didn't set it, failing typecheck once combined with this branch's changes. adaptAlbumChildTile has real is_password_required data but no per-viewer unlock state, so is_locked mirrors is_password_required (assume still locked rather than claim unlocked without evidence). adaptCategoryTile already hardcodes is_password_required to false for these four listings, so is_locked follows the same false default.
5820dfd to
8b83f5d
Compare
2 participants
Adds a "Show cover when locked" toggle to the Visibility tab (v7 and
v8) for password-protected albums. When enabled, the album's cover
thumbnail is shown to anonymous visitors on gallery listings even
before they enter the password — the photos themselves stay hidden
until unlock. This replaces the need for a custom-JS workaround some
users (myself included) had resorted to.
change unless an admin explicitly opts an album in.
ThumbAlbumResource. The embed endpoint is intentionally out of
scope for this PR: EmbededRequest::authorize() already rejects any
password-protected album outright before the resource layer is
ever reached, and the embed DTO doesn't carry a cover/thumbnail
concept in the first place. Wiring cover-while-locked into that
flow is a separate design decision, not a small addition.
AlbumListItem.vue (v7 + v8) inferred "is this album locked" from
thumb === null, which was only ever true by coincidence — theold ThumbAlbumResource had exactly one code path that nulled out
thumb, and it was the same path that ran when the album waslocked, so the two facts always lined up. This PR breaks that
coincidence on purpose: a locked album with the new toggle on now
has
is_password_required = trueand a non-nullthumbat thesame time. Run that state through the old badge logic and it shows
the green "unlocked" badge on an album that still requires a
password to view any photos — a real, user-visible bug that simply
had no way to occur before now. Fixed by adding an explicit
is_lockedfield on ThumbAlbumResource and pointing all fourcomponents at it instead of inferring from
thumb.Note for anyone driving the API directly:
grants_cover_accessisnow a required field on
Album::updateProtectionPolicy, matchingthe existing validation on
grants_full_photo_access.Tests added:
AlbumsTest::testLockedAlbumHidesThumbByDefaultand::testLockedAlbumWithGrantsCoverAccessShowsThumb, plus updates toexisting protection-policy tests for the new required field.
Summary by CodeRabbit
New Features
Bug Fixes