Allow non-authenticated users to star photo (by admin option) #4082
Conversation
Added translations for Bulgarian language.
… for photos selection. Added migration for config. - Added functionality to copy to clipboard names of selected images.
📝 WalkthroughWalkthroughAdds photo-starring support: new enum and config, policies/trait for CAN_STAR, migration for visibility setting, API resources exposing star rights, frontend UI/events and state for starring/filtering, context-menu permission updates, date formatting tweak, and many translation additions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
resources/js/stores/PhotosState.ts (2)
18-22:⚠️ Potential issue | 🟡 Minor
reset()doesn't clearallPhotosorstarredPhotosCount.After
reset(),allPhotosandstarredPhotosCountbecome stale. They should be reset alongsidephotos.Suggested fix
reset() { this.photos = []; + this.allPhotos = []; this.photosTimeline = undefined; this.photoRatingFilter = null; + this.starredPhotosCount = 0; },
67-109:⚠️ Potential issue | 🟠 Major
appendPhotosdoesn't updateallPhotosorstarredPhotosCount.When additional pages of photos are loaded via
appendPhotos, theallPhotoscache andstarredPhotosCountare not updated. This means:
- Toggling the starred filter after paginating will lose the newly appended photos.
- The starred count in the header badge will be wrong.
Suggested fix (add at the end of `appendPhotos`)
appendPhotos(photos: App.Http.Resources.Models.PhotoResource[], isTimeline: boolean) { // ... existing code ... + this.allPhotos = this.photos; + this.starredPhotosCount = this.photos.filter((p) => p.is_starred).length; },
🧹 Nitpick comments (5)
lang/cz/all_settings.php (1)
17-17: Minor grammar nit in translation string."set star flag to photo" reads more naturally as "set the star flag on a photo". This likely affects all locale files that share the same English fallback.
Suggested wording
- 'photos_star_visibility' => 'Who can see and set star flag to photo', + 'photos_star_visibility' => 'Who can see and set the star flag on a photo',lang/cz/dialogs.php (1)
253-255: Array keys use camelCase, inconsistent with the rest of the file.Every other top-level key in this file uses
snake_case(e.g.,share_album,move_photo,photo_delete), but the new group uses camelCase (selectedImages→namesCopied). To stay consistent:Proposed fix
- 'selectedImages' => [ - 'namesCopied' => 'The names of the selected images have been copied!', + 'selected_images' => [ + 'names_copied' => 'The names of the selected images have been copied!', ],Ensure all references to these keys (in Blade/Vue templates) are updated accordingly.
#!/bin/bash # Find all references to the camelCase keys in the codebase rg -rn 'selectedImages' --type-add 'vue:*.vue' --type-add 'blade:*.blade.php' -g '!lang/' rg -rn 'namesCopied' --type-add 'vue:*.vue' --type-add 'blade:*.blade.php' -g '!lang/' # Also check other lang files for consistency rg -rn 'selectedImages' lang/lang/en/dialogs.php (1)
256-258: Inconsistent key naming: camelCase vs snake_case.All other keys in this file use
snake_case(e.g.,share_album,url_copied,move_album,photo_delete). These new keys use camelCase, which breaks the established convention. Consider renaming toselected_imagesandnames_copiedfor consistency across all locale files.♻️ Suggested fix
- 'selectedImages' => [ - 'namesCopied' => 'The names of the selected images have been copied!', + 'selected_images' => [ + 'names_copied' => 'The names of the selected images have been copied!', ],resources/js/components/headers/AlbumHeader.vue (1)
29-36: "Copy starred names" button semantics are unclear.The
pi-cloneicon andcan_editguard don't clearly convey that this button copies starred photo names to clipboard. Consider:
- Using
pi-copyorpi-clipboardicon instead.- Adding a
:titletooltip explaining the action.- Reviewing whether
can_editis the right permission gate (the action reads data, it doesn't edit anything).database/migrations/2026_02_11_170000_add_star_photo_visibility_config.php (1)
9-10: Missinguseimport for theDBfacade.
DBis referenced on lines 44 and 55 but is not explicitly imported. While Laravel's global alias typically resolves this, adding the import is consistent with PSR-4 and avoids surprises if aliases are ever disabled.Proposed fix
use Illuminate\Database\Migrations\Migration; +use Illuminate\Support\Facades\DB;
| /** | ||
| * Checks whether the photos in the album can be starred by the current user. | ||
| * | ||
| * An album is called _starable_ if the current user is allowed to star | ||
| * the album's photos. | ||
| * This also covers adding new photos to an album. | ||
| * An album is _starable_ if any of the following conditions hold | ||
| * (OR-clause) | ||
| * | ||
| * - the user is an admin | ||
| * - the user is the owner of the album | ||
| * | ||
| * @param User $user | ||
| * @param BaseAlbum $album the album; `null` designates the root album | ||
| * | ||
| * @return bool | ||
| */ | ||
| public function canStar(?User $user, BaseAlbum $album): bool | ||
| { | ||
| if ($this->isOwner($user, $album)) { | ||
| return true; | ||
| } | ||
|
|
||
| $config_manager = app(ConfigManager::class); | ||
| if ($user !== null && $config_manager->getValueAsString('photos_star_visibility') === 'authenticated') { | ||
| return true; | ||
| } | ||
|
|
||
| if ($user === null && $config_manager->getValueAsString('photos_star_visibility') === 'anonymous' && $album->public_permissions() !== null) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
PHPDoc mismatch and consider using the PhotoVisibilityType enum instead of raw strings.
- Line 641: The
@paramsaysUser $userbut the signature accepts?User $user. - Lines 653 and 657 compare against raw strings
'authenticated'and'anonymous'. Since the PR introducesPhotoVisibilityTypeenum, use it for type-safe comparison. - Line 653: Any authenticated user can star photos in any album regardless of access rights. If this is intentional, a brief comment would help; otherwise consider gating on
canAccess.
Suggested fix for items 1 & 2
/**
* Checks whether the photos in the album can be starred by the current user.
- * ...
- * `@param` User $user
+ * ...
+ * `@param` User|null $user
* `@param` BaseAlbum $album the album; `null` designates the root album
*
* `@return` bool
*/
public function canStar(?User $user, BaseAlbum $album): bool
{
if ($this->isOwner($user, $album)) {
return true;
}
+ $config_manager = app(ConfigManager::class);
+ $visibility = $config_manager->getValueAsEnum('photos_star_visibility', PhotoVisibilityType::class);
+
- $config_manager = app(ConfigManager::class);
- if ($user !== null && $config_manager->getValueAsString('photos_star_visibility') === 'authenticated') {
+ if ($user !== null && $visibility === PhotoVisibilityType::AUTHENTICATED) {
return true;
}
- if ($user === null && $config_manager->getValueAsString('photos_star_visibility') === 'anonymous' && $album->public_permissions() !== null) {
+ if ($user === null && $visibility === PhotoVisibilityType::ANONYMOUS && $album->public_permissions() !== null) {
return true;
}
return false;
}You'll also need to add the import at the top of the file:
use App\Enum\PhotoVisibilityType;| /** | ||
| * Checks whether the photo can be starred by the current user. | ||
| * | ||
| * A photo is called _starred_ if the current user is allowed to star | ||
| * the photo. | ||
| * A photo is _starred_ if any of the following conditions hold | ||
| * (OR-clause) | ||
| * | ||
| * - the user is an admin | ||
| * - the user is the owner of the photo | ||
| * | ||
| * @param User|null $user | ||
| * @param Photo $photo | ||
| * | ||
| * @return bool | ||
| */ |
There was a problem hiding this comment.
Docblock is inaccurate — it omits the album-level visibility conditions.
The PHPDoc states only "the user is an admin" and "the user is the owner" as conditions, but the implementation also delegates to AlbumPolicy::canStar which grants access based on photos_star_visibility config for authenticated and anonymous users. Update the docblock to reflect the full set of conditions.
Suggested docblock update
/**
* Checks whether the photo can be starred by the current user.
*
- * A photo is called _starred_ if the current user is allowed to star
- * the photo.
- * A photo is _starred_ if any of the following conditions hold
+ * A photo can be starred if any of the following conditions hold
* (OR-clause)
*
* - the user is an admin
* - the user is the owner of the photo
+ * - the photo belongs to an album where starring is allowed per the album policy
+ * (based on photos_star_visibility config)
*
* `@param` User|null $user
* `@param` Photo $photo
*
* `@return` bool
*/| toggleStarred: () => { | ||
| if (!albumStore.album?.id) return; | ||
| if (albumStore.showStarredOnly) { | ||
| albumStore.setShowStarredOnly(false); | ||
| photosStore.filterPhotos(null); | ||
| } else { | ||
| albumStore.setShowStarredOnly(true); | ||
| photosStore.filterPhotos({ is_starred: true }); | ||
| } | ||
|
|
||
| unselect(); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f -e ts -e tsx "photosStore" | head -20Repository: LycheeOrg/Lychee
Length of output: 42
🏁 Script executed:
rg -l "filterPhotos" --type=ts --type=tsx --type=vue | head -10Repository: LycheeOrg/Lychee
Length of output: 87
🏁 Script executed:
rg -n "export.*filterPhotos" --type=ts --type=tsx -B2 -A15Repository: LycheeOrg/Lychee
Length of output: 87
🏁 Script executed:
fd -t f -e ts "photosStore" -o -name "*store*" | grep -i photoRepository: LycheeOrg/Lychee
Length of output: 290
🏁 Script executed:
rg "filterPhotos" -t ts -t vue -n | head -20Repository: LycheeOrg/Lychee
Length of output: 87
🏁 Script executed:
fd -t f -e ts | xargs grep -l "filterPhotos" | head -10Repository: LycheeOrg/Lychee
Length of output: 94
🏁 Script executed:
wc -l resources/js/stores/PhotosState.tsRepository: LycheeOrg/Lychee
Length of output: 98
🏁 Script executed:
rg "filterPhotos" -B5 -A20 resources/js/stores/PhotosState.tsRepository: LycheeOrg/Lychee
Length of output: 644
🏁 Script executed:
rg "allPhotos|filteredPhotos|filteredPhotosTimeline" -B2 -A2 resources/js/stores/PhotosState.ts | head -80Repository: LycheeOrg/Lychee
Length of output: 1094
🏁 Script executed:
cat -n resources/js/stores/PhotosState.tsRepository: LycheeOrg/Lychee
Length of output: 7249
🏁 Script executed:
rg "setPhotos" -B5 -A20 resources/js/stores/PhotosState.tsRepository: LycheeOrg/Lychee
Length of output: 1054
🏁 Script executed:
rg -n "toggleStarred" resources/js/components/gallery/albumModule/AlbumPanel.vue -B5 -A15Repository: LycheeOrg/Lychee
Length of output: 1869
🏁 Script executed:
wc -l resources/js/components/gallery/albumModule/AlbumPanel.vueRepository: LycheeOrg/Lychee
Length of output: 122
🏁 Script executed:
rg -n "showStarredOnly" resources/js/stores/ -B2 -A2Repository: LycheeOrg/Lychee
Length of output: 1333
🏁 Script executed:
rg -n "filteredPhotosTimeline\|filteredPhotos" resources/js/components/gallery/albumModule/AlbumPanel.vue -B3 -A3Repository: LycheeOrg/Lychee
Length of output: 42
🏁 Script executed:
fd -t f -name "PhotoThumbPanel*"Repository: LycheeOrg/Lychee
Length of output: 290
🏁 Script executed:
fd PhotoThumbPanelRepository: LycheeOrg/Lychee
Length of output: 262
🏁 Script executed:
grep -n "PhotoThumbPanel\|filteredPhotos\|filteredPhotosTimeline" resources/js/components/gallery/albumModule/AlbumPanel.vueRepository: LycheeOrg/Lychee
Length of output: 278
🏁 Script executed:
sed -n '1,100p' resources/js/components/gallery/albumModule/AlbumPanel.vueRepository: LycheeOrg/Lychee
Length of output: 3999
🏁 Script executed:
head -100 resources/js/components/gallery/albumModule/PhotoThumbPanel.vueRepository: LycheeOrg/Lychee
Length of output: 3779
🏁 Script executed:
grep -n "photos-timeline\|props\|filteredPhotosTimeline" resources/js/components/gallery/albumModule/PhotoThumbPanel.vue | head -20Repository: LycheeOrg/Lychee
Length of output: 817
🏁 Script executed:
rg "is_photo_timeline_enabled" resources/js/components/gallery/albumModule/PhotoThumbPanel.vue -B5 -A10Repository: LycheeOrg/Lychee
Length of output: 42
🏁 Script executed:
cat resources/js/components/gallery/albumModule/PhotoThumbPanel.vue | tail -60Repository: LycheeOrg/Lychee
Length of output: 2192
🏁 Script executed:
rg "is_photo_timeline_enabled" resources/js/stores/AlbumState.ts -B2 -A2Repository: LycheeOrg/Lychee
Length of output: 422
🏁 Script executed:
rg "photosTimeline.*=" resources/js/stores/PhotosState.ts | head -20Repository: LycheeOrg/Lychee
Length of output: 348
🏁 Script executed:
rg "filterPhotos" resources/js/components/ -lRepository: LycheeOrg/Lychee
Length of output: 118
When starred filter is active in timeline mode, the timeline structure shows all photos instead of only starred photos.
When filterPhotos({ is_starred: true }) filters this.photos, it does not update photosTimeline. Since filteredPhotosTimeline returns the rating-filtered timeline (not applying the starred filter), the timeline view in PhotoThumbPanel displays all photos while the flat list contains only starred ones, creating a display mismatch.
The ordering itself is preserved because allPhotos contains the merged timeline result, but the filter inconsistency between the flat and timeline views needs to be addressed—either by updating photosTimeline when filtering or by applying the same filter to filteredPhotosTimeline.
| copyStarred: () => { | ||
| const selected: any = photosStore.photos.filter((p) => p.is_starred); | ||
| const selectedNames = selected.map((p) => p.title.split(".")[0]).join(", "); | ||
| navigator.clipboard.writeText(selectedNames).then(() => | ||
| toast.add({ | ||
| severity: "info", | ||
| summary: "Info", | ||
| detail: trans("dialogs.selectedImages.namesCopied") + ". " + selectedNames, | ||
| life: 3000, | ||
| }), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Fix any type (CI failure) and add clipboard error handling.
- The static analysis CI flags
anyon line 347. Use the proper type. navigator.clipboard.writeTextcan reject (insecure context, permission denied) — the promise lacks a.catch().p.title.split(".")[0]is fragile for titles with multiple dots (e.g.,"photo.2024.jpg"→"photo"). Considerp.title.replace(/\.[^.]+$/, "")to strip only the last extension.
Suggested fix
copyStarred: () => {
- const selected: any = photosStore.photos.filter((p) => p.is_starred);
- const selectedNames = selected.map((p) => p.title.split(".")[0]).join(", ");
- navigator.clipboard.writeText(selectedNames).then(() =>
+ const starred = photosStore.photos.filter((p) => p.is_starred);
+ const selectedNames = starred.map((p) => p.title.replace(/\.[^.]+$/, "")).join(", ");
+ navigator.clipboard.writeText(selectedNames).then(() => {
toast.add({
severity: "info",
summary: "Info",
detail: trans("dialogs.selectedImages.namesCopied") + ". " + selectedNames,
life: 3000,
- }),
- );
+ });
+ }).catch(() => {
+ toast.add({
+ severity: "error",
+ summary: "Error",
+ detail: "Failed to copy to clipboard",
+ life: 3000,
+ });
+ });
},📝 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.
| copyStarred: () => { | |
| const selected: any = photosStore.photos.filter((p) => p.is_starred); | |
| const selectedNames = selected.map((p) => p.title.split(".")[0]).join(", "); | |
| navigator.clipboard.writeText(selectedNames).then(() => | |
| toast.add({ | |
| severity: "info", | |
| summary: "Info", | |
| detail: trans("dialogs.selectedImages.namesCopied") + ". " + selectedNames, | |
| life: 3000, | |
| }), | |
| ); | |
| }, | |
| copyStarred: () => { | |
| const starred = photosStore.photos.filter((p) => p.is_starred); | |
| const selectedNames = starred.map((p) => p.title.replace(/\.[^.]+$/, "")).join(", "); | |
| navigator.clipboard.writeText(selectedNames).then(() => { | |
| toast.add({ | |
| severity: "info", | |
| summary: "Info", | |
| detail: trans("dialogs.selectedImages.namesCopied") + ". " + selectedNames, | |
| life: 3000, | |
| }); | |
| }).catch(() => { | |
| toast.add({ | |
| severity: "error", | |
| summary: "Error", | |
| detail: "Failed to copy to clipboard", | |
| life: 3000, | |
| }); | |
| }); | |
| }, |
🧰 Tools
🪛 GitHub Check: 0️⃣ JS front-end / Node 22
[failure] 347-347:
Unexpected any. Specify a different type
| <Button | ||
| v-if="photos_star_visibility === 'anonymous' || (photos_star_visibility === 'authenticated' && userStore.isLoggedIn)" | ||
| :title="'Show starred images'" | ||
| :icon="albumStore.showStarredOnly ? 'pi pi-star-fill' : 'pi pi-star'" | ||
| :label="String(photosStore.starredPhotosCount)" | ||
| class="border-none hover:text-color" | ||
| severity="secondary" | ||
| text | ||
| @click="emits('showStarredImages')" | ||
| /> |
There was a problem hiding this comment.
Hardcoded English string — use $t() for i18n.
Line 21 uses a hardcoded 'Show starred images' title instead of a translated string. This will display English regardless of the user's locale.
- :title="'Show starred images'"
+ :title="$t('gallery.album.show_starred_images')"📝 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.
| <Button | |
| v-if="photos_star_visibility === 'anonymous' || (photos_star_visibility === 'authenticated' && userStore.isLoggedIn)" | |
| :title="'Show starred images'" | |
| :icon="albumStore.showStarredOnly ? 'pi pi-star-fill' : 'pi pi-star'" | |
| :label="String(photosStore.starredPhotosCount)" | |
| class="border-none hover:text-color" | |
| severity="secondary" | |
| text | |
| @click="emits('showStarredImages')" | |
| /> | |
| <Button | |
| v-if="photos_star_visibility === 'anonymous' || (photos_star_visibility === 'authenticated' && userStore.isLoggedIn)" | |
| :title="$t('gallery.album.show_starred_images')" | |
| :icon="albumStore.showStarredOnly ? 'pi pi-star-fill' : 'pi pi-star'" | |
| :label="String(photosStore.starredPhotosCount)" | |
| class="border-none hover:text-color" | |
| severity="secondary" | |
| text | |
| `@click`="emits('showStarredImages')" | |
| /> |
| filterPhotos(filter: any) { | ||
| this.photos = this.allPhotos.filter((photo) => { | ||
| if (!filter) { | ||
| return photo; | ||
| } | ||
|
|
||
| for (const key in filter) { | ||
| if ((photo as any)[key] !== filter[key]) { | ||
| return false; | ||
| } | ||
| } | ||
| return photo; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Fix any types (CI failure) and filter callback return values.
The CI fails on lines 110 and 117 due to any. Define a proper filter type. Also, return photo in a filter predicate is truthy but semantically should be return true.
Suggested fix
- filterPhotos(filter: any) {
- this.photos = this.allPhotos.filter((photo) => {
- if (!filter) {
- return photo;
- }
-
- for (const key in filter) {
- if ((photo as any)[key] !== filter[key]) {
- return false;
- }
- }
- return photo;
- });
+ filterPhotos(filter: Partial<App.Http.Resources.Models.PhotoResource> | null) {
+ if (!filter) {
+ this.photos = this.allPhotos;
+ return;
+ }
+ this.photos = this.allPhotos.filter((photo) => {
+ for (const key in filter) {
+ if (photo[key as keyof App.Http.Resources.Models.PhotoResource] !== filter[key as keyof App.Http.Resources.Models.PhotoResource]) {
+ return false;
+ }
+ }
+ return true;
+ });
},📝 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.
| filterPhotos(filter: any) { | |
| this.photos = this.allPhotos.filter((photo) => { | |
| if (!filter) { | |
| return photo; | |
| } | |
| for (const key in filter) { | |
| if ((photo as any)[key] !== filter[key]) { | |
| return false; | |
| } | |
| } | |
| return photo; | |
| }); | |
| }, | |
| filterPhotos(filter: Partial<App.Http.Resources.Models.PhotoResource> | null) { | |
| if (!filter) { | |
| this.photos = this.allPhotos; | |
| return; | |
| } | |
| this.photos = this.allPhotos.filter((photo) => { | |
| for (const key in filter) { | |
| if (photo[key as keyof App.Http.Resources.Models.PhotoResource] !== filter[key as keyof App.Http.Resources.Models.PhotoResource]) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| }); | |
| }, |
🧰 Tools
🪛 GitHub Check: 0️⃣ JS front-end / Node 22
[failure] 117-117:
Unexpected any. Specify a different type
[failure] 110-110:
Unexpected any. Specify a different type
| * @return bool | ||
| */ | ||
| public function canStar(?User $user, AbstractAlbum $album): bool | ||
| { |
There was a problem hiding this comment.
Indentation inconsistency: spaces instead of tabs.
Line 647 uses spaces for the opening brace, while the rest of the file uses tabs. Run php-cs-fixer fix to normalize. As per coding guidelines: "Run vendor/bin/php-cs-fixer fix to apply PHP code style fixes before committing."
public function canStar(?User $user, AbstractAlbum $album): bool
- {
+ {2 participants
Summary by CodeRabbit
New Features
Documentation
Permissions