Conversation
Added translations for Bulgarian language.
…header_improvements
…r persistence. Add "Edit", "Save", "Open Gallery", and view layout buttons to album hero. Fix is_album_enhanced_display_enabled and album_header_size config loading. Ensure album properties sync correctly by clearing cache and reloading store on save. Add and propagate translations for new header options and actions to all languages.
Fixes for unit tests run.
…header_improvements # Conflicts: # app/Http/Middleware/ConfigIntegrity.php # resources/js/components/gallery/albumModule/AlbumPanel.vue # resources/js/lychee.d.ts
|
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:
📝 WalkthroughWalkthroughAdds enhanced album display: header size config and enum, title color/position enums and storage, header focus storage and clamping, header retrieval now returns SizeVariant with palette, frontend components for header/title/focus, migrations, translations, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Http/Controllers/Gallery/AlbumController.php (1)
295-301:⚠️ Potential issue | 🟠 Major
header_photo_focusshould not be cleared when setting the cover.
cover_id(the album thumbnail) andheader_id(the hero/header image) are unrelated fields. Resettingheader_photo_focushere discards a user's saved header focus point whenever they change the cover — they are completely independent. This line appears to be an accidental addition (possibly copy-pasted from another location that does legitimately clear focus).🐛 Proposed fix
public function cover(SetAsCoverRequest $request): void { $album = $request->album(); $album->cover_id = ($album->cover_id === $request->photo()->id) ? null : $request->photo()->id; - $album->header_photo_focus = null; $album->save(); }resources/js/components/forms/album/AlbumProperties.vue (1)
110-187:⚠️ Potential issue | 🟡 MinorMissing
flex-wrapmay cause horizontal overflow with three selects.The container
divat line 110 usesflexwithoutflex-wrap, but now holds up to three Select components (headerw-90+ title stylew-62+ title positionw-62≈ 53.5 rem total). The card's minimum width issm:min-w-3xl(48 rem), so these will overflow on narrower viewports. Compare with line 218 which correctly usesflex flex-wrap gap-y-4for a similar multi-select row.🔧 Suggested fix
- <div class="h-10 my-2 flex"> + <div class="sm:h-10 my-2 flex flex-wrap gap-y-4">
🟡 Minor comments (14)
lang/cz/gallery.php-124-127 (1)
124-127:⚠️ Potential issue | 🟡 Minor
'Open gallery'capitalization is inconsistent with the surrounding hero strings.All other entries in this section (
'Download Album','Share Album','Embed Album') use title case.'Open gallery'should be'Open Gallery'to match.✏️ Proposed fix
- 'open_gallery' => 'Open gallery', + 'open_gallery' => 'Open Gallery',config/database.php-110-113 (1)
110-113:⚠️ Potential issue | 🟡 MinorUse
\PDO::for generic PDO attributes, not\Pdo\Mysql::Lines 111–112 use
\Pdo\Mysql::ATTR_TIMEOUTand\Pdo\Mysql::ATTR_PERSISTENT, but these are generic PDO attributes, not MySQL-specific. While they resolve correctly through inheritance, the idiomatic form is to use\PDO::ATTR_TIMEOUTand\PDO::ATTR_PERSISTENT. Reserve\Pdo\Mysql::for true MySQL-specific constants likeATTR_SSL_CAandATTR_INIT_COMMAND.Proposed fix
'options' => extension_loaded('pdo_mysql') ? array_filter([ \Pdo\Mysql::ATTR_SSL_CA => env('MYSQL_ATTR_SSL_CA'), - \Pdo\Mysql::ATTR_TIMEOUT => 5, // Connection timeout - \Pdo\Mysql::ATTR_PERSISTENT => false, // NEVER use persistent connections with Octane + \PDO::ATTR_TIMEOUT => 5, // Connection timeout + \PDO::ATTR_PERSISTENT => false, // NEVER use persistent connections with Octane \Pdo\Mysql::ATTR_INIT_COMMAND => 'SET SESSION wait_timeout=28800', // 8 hours ], fn ($v) => $v !== '' && $v !== null && $v !== []) : [],lang/it/toasts.php-11-12 (1)
11-12:⚠️ Potential issue | 🟡 MinorNew toast keys are not translated to Italian.
The existing
error/successkeys were already English in this file; these additions continue that pattern. Consider a translation pass for all keys in a follow-up.lang/es/toasts.php-10-11 (1)
10-11:⚠️ Potential issue | 🟡 MinorNew toast keys are not translated to Spanish.
successis properly localized as'Éxito'while the new entries remain in English.🌐 Suggested Spanish translations
- 'album_updated' => 'Album updated', - 'update_failed' => 'Failed to update album', + 'album_updated' => 'Álbum actualizado', + 'update_failed' => 'Error al actualizar el álbum',lang/cz/toasts.php-10-11 (1)
10-11:⚠️ Potential issue | 🟡 MinorNew toast keys are not translated to Czech.
album_updatedandupdate_failedare English in a Czech file whereerroris already properly localized as'Chyba'.🌐 Suggested Czech translations
- 'album_updated' => 'Album updated', - 'update_failed' => 'Failed to update album', + 'album_updated' => 'Album byl aktualizován', + 'update_failed' => 'Aktualizace alba se nezdařila',database/migrations/2026_02_16_165731_add_enhanced_display_fields_to_albums.php-19-34 (1)
19-34:⚠️ Potential issue | 🟡 Minor
down()will fail on rollback whenup()was skipped by the guard.
up()only adds columns whentitle_colordoesn't exist, butdown()drops them unconditionally. If another migration had already addedtitle_colorbefore this one ran (causingup()to no-op), aphp artisan migrate:rollbackwould attempt to drop non-existent columns and throw an exception.Apply a symmetric guard to
down():🐛 Proposed fix
public function down(): void { + if (!Schema::hasColumn('albums', 'title_color')) { + return; + } Schema::table('albums', function (Blueprint $table) { $table->dropColumn(['title_color', 'title_position']); }); }resources/js/components/gallery/albumModule/HeaderEditButton.vue-2-4 (1)
2-4:⚠️ Potential issue | 🟡 MinorMissing
type="button"andaria-labelon the icon-only button.Without
type="button", if this component is ever rendered inside a<form>it defaults totype="submit". Additionally, a button containing only an<i>icon has no accessible name for screen readers.🛡️ Proposed fix
-<button :class="['h-8', 'w-1/5', 'cursor-pointer hover:bg-white/30 hover:rounded-md']" `@click`="emits('setPosition', props.position)"> +<button + type="button" + :aria-label="props.position" + :class="['h-8', 'w-1/5', 'cursor-pointer hover:bg-white/30 hover:rounded-md']" + `@click`="emits('setPosition', props.position)" +>resources/js/components/gallery/albumModule/HeaderFocusPicker.vue-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorDuplicate
z-indexclasses:z-50andz-100.The outer
<div>has bothz-50andz-100. Only one should be used — the second overrides the first depending on stylesheet order.🔧 Proposed fix
- <div class="absolute top-10 right-1 z-50 w-72 bg-surface-800 rounded-lg shadow-xl overflow-hidden border border-surface-700 z-100"> + <div class="absolute top-10 right-1 z-100 w-72 bg-surface-800 rounded-lg shadow-xl overflow-hidden border border-surface-700">resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue-80-83 (1)
80-83:⚠️ Potential issue | 🟡 MinorConflicting
relativeandabsolutepositioning on the same element.Line 82 has both
relativeandabsoluteclasses. Since CSS applies them based on specificity/order,absolutewill effectively win, but having both is confusing and likely unintentional. Removerelativeifabsoluteis intended.resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue-128-128 (1)
128-128:⚠️ Potential issue | 🟡 MinorRemove unused
useTemplateRefimport (pipeline failure).
useTemplateRefis imported but never used, causing the lint check to fail.Proposed fix
-import { ref, computed, useTemplateRef, onMounted, watch } from "vue"; +import { ref, computed, onMounted, watch } from "vue";resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue-104-106 (1)
104-106:⚠️ Potential issue | 🟡 Minor
t-4is not a valid Tailwind utility — likely should bemt-4.On line 105,
t-4is not a recognized Tailwind CSS class. This looks like a typo formt-4(margin-top).Proposed fix
- class="t-4 sm:mt-2 inline-block px-6 py-3 sm:px-10 sm:py-4 lg:px-12 lg:py-4 text-2xl sm:text-sm lg:text-xs font-semibold tracking-wide sm:tracking-wider uppercase cursor-pointer hover:bg-white/10 w-fit border" + class="mt-4 sm:mt-2 inline-block px-6 py-3 sm:px-10 sm:py-4 lg:px-12 lg:py-4 text-2xl sm:text-sm lg:text-xs font-semibold tracking-wide sm:tracking-wider uppercase cursor-pointer hover:bg-white/10 w-fit border"resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue-56-78 (1)
56-78:⚠️ Potential issue | 🟡 Minor
-translate-y-0has no visual effect — likely should be-translate-y-1/2for vertical centering.Lines 62 and 74 use
-translate-y-0which translates totransform: translateY(0), which does nothing. Given thetop-1/2positioning,-translate-y-1/2is typically used to vertically center an element.Proposed fix
- 'top-1/2 -left-16 -translate-y-0 absolute text-white', + 'top-1/2 -left-16 -translate-y-1/2 absolute text-white',- 'top-1/2 -right-16 -translate-y-0 absolute text-white', + 'top-1/2 -right-16 -translate-y-1/2 absolute text-white',resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue-151-153 (1)
151-153:⚠️ Potential issue | 🟡 MinorAvoid
any— type thealbumprop (pipeline failure).The
album: anyprop and theas anycast on line 164 cause lint failures. Use the actual album resource type.Proposed fix
const props = defineProps<{ - album: any; + album: App.Http.Resources.Models.AlbumResource; }>();And for line 164:
- const p = props.album.preFormattedData.palette as any; + const p = props.album.preFormattedData.palette;resources/js/lychee.d.ts-896-896 (1)
896-896:⚠️ Potential issue | 🟡 Minor
Array<any>should be typed as an object with color properties.The palette is constructed as an associative array with string color values (hex codes), not an array. The TypeScript type should reflect the actual data structure from
PreFormattedAlbumData.php:Proposed fix
- palette: Array<any> | null; + palette: { color1: string; color2: string; color3: string; color4: string; color5: string } | null;
🧹 Nitpick comments (24)
lang/it/gallery.php (1)
160-175: Nit: Extra leading space in nested array indentation.The keys inside
position_options(lines 161–165) andstyle_options(lines 168–174) use 5-space indentation instead of the 4-space convention used throughout the rest of the file.♻️ Proposed fix
'position_options' => [ - 'top_left' => 'Top Left', - 'top_right' => 'Top Right', - 'center' => 'Center', - 'bottom_left' => 'Bottom Left', - 'bottom_right' => 'Bottom Right', + 'top_left' => 'Top Left', + 'top_right' => 'Top Right', + 'center' => 'Center', + 'bottom_left' => 'Bottom Left', + 'bottom_right' => 'Bottom Right', ], 'style_options' => [ - 'white' => 'White', - 'black' => 'Black', - 'color_1' => 'Color 1', - 'color_2' => 'Color 2', - 'color_3' => 'Color 3', - 'color_4' => 'Color 4', - 'color_5' => 'Color 5', + 'white' => 'White', + 'black' => 'Black', + 'color_1' => 'Color 1', + 'color_2' => 'Color 2', + 'color_3' => 'Color 3', + 'color_4' => 'Color 4', + 'color_5' => 'Color 5', ],lang/fr/gallery.php (1)
159-174: Extra leading space insideposition_optionsandstyle_optionsarrays.Lines 160–164 and 167–173 each carry an extra leading space before the key (
·'top_left'instead of'top_left'), which breaks the indentation consistency of the rest of the file.🧹 Proposed indentation fix
'position_options' => [ - 'top_left' => '...', - 'top_right' => '...', - 'center' => '...', - 'bottom_left' => '...', - 'bottom_right' => '...', + 'top_left' => '...', + 'top_right' => '...', + 'center' => '...', + 'bottom_left' => '...', + 'bottom_right' => '...', ], 'style_options' => [ - 'white' => '...', - 'black' => '...', - 'color_1' => '...', - 'color_2' => '...', - 'color_3' => '...', - 'color_4' => '...', - 'color_5' => '...', + 'white' => '...', + 'black' => '...', + 'color_1' => '...', + 'color_2' => '...', + 'color_3' => '...', + 'color_4' => '...', + 'color_5' => '...', ],package.json (2)
52-52:@vitejs/plugin-vuebelongs indevDependencies
@vitejs/plugin-vue(latest:6.0.4) is the official Vite plugin for Vue SFC support — a pure build-time tool that is never shipped to or executed in the browser. Listing it underdependenciesinflates the production install graph and may confuse tools that differentiate runtime from build-time deps.♻️ Proposed fix
- "@vitejs/plugin-vue": "^6.0.4", + // ... keep in dependencies only runtime packages ...In
devDependencies:+ "@vitejs/plugin-vue": "^6.0.4",
57-57: Heads-up:image-focusis unmaintained and injects inline styles that may conflict with Tailwind
image-focusv1.2.1 is the latest version, last published 4 years ago, with only 7 other dependents in the npm registry. With no active upstream maintenance, any future vulnerability would require a fork or replacement.Additionally, the library sets
overflow: hiddenon the container andposition: absoluteon the<img>, and warns that if anything appears to be behaving unexpectedly, inline styles should be checked against CSS using!important. Tailwind's utility classes — especially when used with the!important modifier — can silently override these, breaking the focus-point positioning.Please verify that the focus-picker component works correctly alongside the Tailwind styles applied to the album header image and its container.
resources/js/components/gallery/albumModule/AlbumPanel.vue (1)
377-386: Move thephotoPanelref declaration abovealbumCallbacks.
photoPanelis declared at line 386, but it is referenced insidealbumCallbacks.scrollToPaginatorTop(line 378) which is defined starting at line 317. While the closure prevents a TDZ error at runtime (the value is only read at call-time, after setup completes), declaring the ref after the object that uses it is misleading and inconsistent with howareStatisticsOpenand similar refs are handled earlier in the file.♻️ Proposed fix — hoist `photoPanel` next to the now-removed `paginatorTop`
-const paginatorTop = ref<HTMLDivElement | null>(null); - const albumCallbacks = { +const photoPanel = ref<ComponentPublicInstance | null>(null); + +const albumCallbacks = { ... scrollToPaginatorTop: () => { if (photoPanel.value) { photoPanel.value.$el.scrollIntoView({ behavior: "smooth" }); } }, }; const computedAlbum = computed(() => albumStore.album); const computedConfig = computed(() => albumStore.config); -const photoPanel = ref<ComponentPublicInstance | null>(null);resources/js/components/gallery/albumModule/HeaderEditButton.vue (1)
21-21: Add a fallback for unknownpositionvalues.
POSITION_ICONS[props.position]returnsundefinedfor any unrecognised key. Addingundefinedto a Vue:classarray is silently ignored (no icon renders), making failures invisible.♻️ Proposed fix
-const positionIcon = computed(() => POSITION_ICONS[props.position]); +const positionIcon = computed(() => POSITION_ICONS[props.position] ?? "pi-circle");database/migrations/2026_02_16_165731_add_enhanced_display_fields_to_albums.php (1)
21-22:->nullable(false)is the default forstring()columns — remove it.
$table->string(...)is non-nullable by default; the explicit->nullable(false)call is redundant and adds noise.♻️ Proposed fix
-$table->string('title_color', 20)->default('white')->after('album_thumb_aspect_ratio')->nullable(false); -$table->string('title_position', 20)->default('top_left')->after('title_color')->nullable(false); +$table->string('title_color', 20)->default('white')->after('album_thumb_aspect_ratio'); +$table->string('title_position', 20)->default('top_left')->after('title_color');lang/zh_CN/gallery.php (1)
14-17: New strings are not translated to Chinese.All newly added keys (
set_focus,set_header_focus,done,cancel,edit,save,open_gallery, and the entiretitleblock) are in English. The rest of the file is largely localized to Chinese. Consider translating these for consistency with the existing Chinese translations in this file, or file an issue to track localization.Also applies to: 124-126, 157-176
lang/ru/gallery.php (1)
13-16: New strings are not translated to Russian.Same as the Chinese file — all new keys remain in English. The surrounding file is well-translated to Russian.
Also applies to: 123-125, 156-175
lang/de/gallery.php (1)
13-16: New strings are not translated to German.Same pattern as the other locale files — all new keys remain in English.
Also applies to: 123-125, 156-175
resources/js/components/gallery/albumModule/HeaderFocusPicker.vue (1)
5-7: Close button lacks accessible label.The
×icon button uses only<i class="pi pi-times">withoutaria-labelor visually-hidden text. Screen readers won't convey its purpose.🔧 Proposed fix
- <button `@click`="$emit('cancel')" class="text-surface-400 hover:text-white transition-colors cursor-pointer"> + <button `@click`="$emit('cancel')" class="text-surface-400 hover:text-white transition-colors cursor-pointer" :aria-label="$t('gallery.cancel')">app/Http/Resources/Models/Utils/PreFormattedAlbumData.php (1)
38-55: Consider using a typed Data class for$paletteinstead of an untyped array.Since
PreFormattedAlbumDatais annotated with#[TypeScript()], the?arraytype will produce a looseRecord<string, any> | nullon the TypeScript side. A dedicated Data class would give you compile-time type safety on both PHP and TypeScript. The codebase already hasColourPaletteResourceas a precedent for this pattern—either reuse it or create a smallerPaletteDataclass with the property names (color1–color5) that match your frontend expectations.Note: The
photo.paletterelationship is already eagerly loaded inHasHeaderUrl::getByQuery()(lines 46, 87), so no N+1 concern here.tests/Feature_v2/Album/AlbumUpdateTest.php (1)
74-98: Consider asserting the new fields are present in the success response.
testUpdateAlbumAuthorizedonly checksassertOk. Adding assertions fortitle_colorandtitle_positionin the response body would confirm the fields are persisted and properly returned byEditableBaseAlbumResource.✅ Example assertion to add
$this->assertOk($response); +$response->assertJson([ + 'title_color' => 'white', + 'title_position' => 'top_left', +]);resources/js/components/gallery/albumModule/AlbumHeaderImage.vue (1)
2-4: Tailwindobject-cover/object-centeron the container<div>have no effect.
object-fitandobject-position(and their Tailwind utilitiesobject-cover,object-center) only apply to replaced elements like<img>and<video>, not to block containers. These two classes on the wrapper<div>are no-ops and can be removed to keep the class list accurate.🧹 Proposed cleanup
-<div class="image-container overflow-hidden absolute block top-0 left-0 w-full h-full object-cover object-center z-0"> +<div class="image-container overflow-hidden absolute block top-0 left-0 w-full h-full z-0">lang/ar/gallery.php (1)
13-16: New localization keys are not translated to Arabic.All newly added keys (
set_focus,set_header_focus,done,cancel,edit,save,open_gallery, and the entiretitleconfiguration block) are in English. This is consistent with the approach in other language files in this PR, but flagging for awareness in case Arabic translations are desired later.Also applies to: 123-125, 156-175
app/Http/Resources/Editable/EditableBaseAlbumResource.php (1)
41-43: Consider using a typed DTO instead of?arrayforheader_photo_focus.The
header_photo_focusproperty is typed as?array, which is loosely typed. Since it represents{x: float, y: float}coordinates (validated as numeric between -1 and 1 inUpdateAlbumRequest), a small DTO would provide better type safety and self-documentation. This is a minor suggestion and the current approach works.lang/sk/gallery.php (1)
14-17: New localization keys are not translated to Slovak.Same as the Arabic locale — all new strings remain in English. Acceptable per the project's established pattern for new UI strings, but noting for completeness.
Also applies to: 124-126, 157-176
tests/Feature_v2/Album/AlbumUpdateFocusTest.php (1)
38-38: Inconsistent assertion style within the same test class.Lines 38 and 97 use
$response->assertOk()(Laravel's TestResponse method), while lines 64 and 67 use$this->assertOk($response)(the CatchFailures trait helper). Pick one style consistently — the trait helper$this->assertOk($response)is the project convention per theCatchFailurestrait.Proposed fix
- $response->assertOk(); + $this->assertOk($response);And at line 97:
- $response->assertOk(); + $this->assertOk($response);resources/js/config/constants.ts (1)
140-156: Unnecessary type assertions — values already match the annotated array element type.The
as App.Enum.AlbumTitlePositionandas App.Enum.AlbumTitleColorcasts are redundant since the string literals conform to the declaredSelectOption<App.Enum.AlbumTitlePosition>[]andSelectOption<App.Enum.AlbumTitleColor>[]types. Other option arrays in this file (e.g.,photoSortingColumnsOptions) don't use explicit casts.resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue (1)
314-317: Side-effect fetch at module level — consider moving intoonMounted.
InitService.fetchLandingData()is called unconditionally during component setup. If this data is only needed whenis_album_enhanced_display_enabledis true, wrapping it would avoid unnecessary network requests.app/Http/Resources/Traits/HasHeaderUrl.php (2)
67-67: Line exceeds readability limits.This line is quite long. Consider breaking the chained
->whereIn(...)onto its own line for readability, whichphp-cs-fixermay or may not handle automatically.♻️ Suggested formatting
- ->where('ratio', '>', 1)->whereIn('type', [SizeVariantType::MEDIUM2X, SizeVariantType::SMALL2X, SizeVariantType::SMALL]); + ->where('ratio', '>', 1) + ->whereIn('type', [SizeVariantType::MEDIUM2X, SizeVariantType::SMALL2X, SizeVariantType::SMALL]);
20-21: Trait nameHasHeaderUrlno longer reflects its return type.The trait now returns
?SizeVariantinstead of a URL string, making the name misleading. Consider renaming toHasHeaderorHasHeaderSizeVariantfor clarity.resources/js/components/forms/album/AlbumProperties.vue (2)
483-509:saveAlbumcorrectly propagates new fields; consider error handling.The new
title_color,title_position, andheader_photo_focusfields are correctly included in the payload. The.then()callback callsalbumStore.loadHead()to refresh the header — good. However, there is no.catch()handler on the promise chain. IfupdateAlbumfails, the toast won't show an error. This is a pre-existing pattern across bothsaveAlbumandsaveTagAlbum, so it may be intentional (handled in the service layer), but worth noting.
531-535:albumStore.loadHead()called for tag album saves.Tag albums don't have header customization fields (the UI is gated by
is_model_album), so reloading the header after a tag album save is unnecessary work. It's harmless but adds a superfluous network request.
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
app/Models/Album.php (1)
446-495:⚠️ Potential issue | 🟠 MajorReturn hex strings for palette colors in
getComputedTitleColor().
Line 492 currently returns raw integer palette values, which breaks CSS color usage and violates the declared string return type. This matches a previously flagged issue.🐛 Suggested fix
- $color = match ($value) { + $color = match ($value) { 'color1' => $palette->colour_1, 'color2' => $palette->colour_2, 'color3' => $palette->colour_3, 'color4' => $palette->colour_4, 'color5' => $palette->colour_5, - default => '#ffffff', + default => null, }; - - return $color; + return $color !== null ? Palette::toHex($color) : '#ffffff';
🧹 Nitpick comments (1)
resources/js/lychee.d.ts (1)
896-899: Consider narrowingtitle_color/title_positionto enum types if payloads remain enum values.
If the API returns enum values (as in EditableBaseAlbumResource), use the enum types for stronger safety. If these are intended to carry computed hex strings, keepingstringis fine.♻️ Suggested type narrowing (if applicable)
- title_color: string | null; - title_position: string | null; + title_color: App.Enum.AlbumTitleColor | null; + title_position: App.Enum.AlbumTitlePosition | null;
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Http/Resources/Models/Utils/PreFormattedAlbumData.phpapp/Models/Album.phpresources/js/components/gallery/albumModule/AlbumHeaderPanel.vueresources/js/components/gallery/albumModule/AlbumPanel.vueresources/js/lychee.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/js/components/gallery/albumModule/AlbumPanel.vue
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue
Outdated
Show resolved
Hide resolved
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue
Outdated
Show resolved
Hide resolved
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue (5)
18-38: Use PrimeVue<Button>components instead of native<button>elements.This was already flagged in a prior review (lines 18–38 and 56–110). As per coding guidelines,
**/*.vuecomponents should use PrimeVue UI components.
167-178: Fallback palette values are not valid CSS colors.Already flagged in a prior review (lines 167–177). Values like
"blue-600"are Tailwind class names, not valid CSS color strings, soselectedColorwill be broken whenpaletteis absent.
212-219:positionis not reset whentitle_positionis absent or unknown.Already flagged in a prior review (lines 212–219). Navigating between albums leaves a stale position value when the incoming album has no
title_position.
266-266:is_compactforced tofalseon every save.Already flagged in a prior review (lines 266–270). This unintentionally resets the user's compact preference on every header save.
318-321:fetchLandingData()called at setup scope instead ofonMounted, and still missing.catch().The missing
.catch()was flagged in a prior review (lines 318–321). Additionally, running the fetch directly in setup scope (not insideonMounted) means: (1) the request fires every time the component is instantiated, with no lifecycle guard; (2) if the component unmounts before the promise resolves, the callback still sets the ref. Move it intoonMountedand add the catch handler.♻️ Suggested refactor
-const initdata = ref<App.Http.Resources.GalleryConfigs.LandingPageResource | undefined>(undefined); -InitService.fetchLandingData().then((data) => { - initdata.value = data.data; -}); +const initdata = ref<App.Http.Resources.GalleryConfigs.LandingPageResource | undefined>(undefined); + +onMounted(() => { + InitService.fetchLandingData() + .then((data) => { + initdata.value = data.data; + }) + .catch((e) => { + console.error(e); + }); +});
🧹 Nitpick comments (1)
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue (1)
6-15: Preferv-elseover a secondv-if.Both branches are always evaluated; using
v-if/v-elseavoids the redundant condition check and makes the mutual-exclusion intent explicit.♻️ Suggested refactor
- <template v-if="!is_album_enhanced_display_enabled"> + <template v-if="!is_album_enhanced_display_enabled"> <!-- non-enhanced content --> - </template> - <template v-if="is_album_enhanced_display_enabled"> + </template> + <template v-else> <!-- enhanced content --> </template>
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vueresources/js/components/gallery/albumModule/AlbumPanel.vueresources/js/composables/album/photoActions.tsresources/js/views/gallery-panels/Search.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/js/components/gallery/albumModule/AlbumPanel.vue
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/js/components/gallery/albumModule/AlbumPanel.vue (1)
271-297:⚠️ Potential issue | 🟠 Major
setAsHeaderdoesn't clearheader_photo_focus— inconsistent withphotoActions.ts.The
setAlbumHeaderinphotoActions.ts(lines 73-78, 87, 92) now clearsheader_photo_focusonmodelAlbum.editable,modelAlbum.preFormattedData,album.editable, andalbum.preFormattedDatawhen the header is toggled. This context-menu version does not, so stale focus coordinates can persist when a user sets a new header via the context menu.🐛 Proposed fix — clear focus in all branches to match photoActions.ts
setAsHeader: () => { if (albumStore.album === undefined) return; PhotoService.setAsHeader(selectedPhoto.value!.id, albumStore.album.id, false); // Update the album's header_id immediately to reflect the change (toggle behavior) const isToggleOff = albumStore.modelAlbum?.header_id === selectedPhoto.value!.id; if (albumStore.modelAlbum !== undefined) { albumStore.modelAlbum.header_id = isToggleOff ? null : selectedPhoto.value!.id; + if (albumStore.modelAlbum.editable) { + albumStore.modelAlbum.editable.header_photo_focus = null; + } + if (albumStore.modelAlbum.preFormattedData) { + albumStore.modelAlbum.preFormattedData.header_photo_focus = null; + } } if ( albumStore.album !== undefined && "editable" in albumStore.album && albumStore.album.editable !== undefined && albumStore.album.editable !== null ) { albumStore.album.editable.header_id = isToggleOff ? null : selectedPhoto.value!.id; + albumStore.album.editable.header_photo_focus = null; } // Update the header image URL in the album's preFormattedData if (albumStore.album.preFormattedData) { + albumStore.album.preFormattedData.header_photo_focus = null; if (isToggleOff) { albumStore.album.preFormattedData.url = null; } else {
♻️ Duplicate comments (5)
resources/js/views/gallery-panels/Search.vue (1)
291-298:⚠️ Potential issue | 🟡 MinorClear editable header focus when toggling header_id.
When the header is toggled off,
header_photo_focusshould be reset alongsideheader_idto avoid stale focus data in the editable model.Suggested fix
if (albumStore.modelAlbum !== undefined) { albumStore.modelAlbum.header_id = isToggleOff ? null : selectedPhoto.value!.id; + albumStore.modelAlbum.header_photo_focus = null; } if ( albumStore.album !== undefined && "editable" in albumStore.album && albumStore.album.editable !== undefined && albumStore.album.editable !== null ) { albumStore.album.editable.header_id = isToggleOff ? null : selectedPhoto.value!.id; + albumStore.album.editable.header_photo_focus = null; }resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue (4)
165-176: Fallback palette values are not valid CSS colors.Values like
"blue-600","red-600", etc. are Tailwind class fragments, not valid CSS color strings. When applied via:style="{ color: selectedColor }", the browser will ignore them. This was flagged in a previous review and remains unaddressed.
270-270:is_compactis hard-coded tofalseon save.This overwrites the user's existing compactness preference whenever they save header customizations. This was flagged in a previous review and remains unaddressed.
53-76: Native<button>elements still used instead of PrimeVueButton.The color-cycling chevrons (lines 53–76) and the "Open Gallery" button (lines 101–107) still use native
<button>rather than PrimeVue<Button>. This was flagged in a previous review. As per coding guidelines,**/*.vue: "Use TypeScript with Vue3 composition API and use PrimeVue for UI components."Also applies to: 101-107
322-325:fetchLandingData()promise has no error handler.An unhandled rejection here will surface as an uncaught promise error. This was flagged in a previous review and remains unaddressed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vueresources/js/components/gallery/albumModule/AlbumPanel.vueresources/js/composables/album/photoActions.tsresources/js/views/gallery-panels/Search.vue
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue
Outdated
Show resolved
Hide resolved
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue
Outdated
Show resolved
Hide resolved
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (7)
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue (7)
53-73:⚠️ Potential issue | 🟡 MinorChevron buttons use
-translate-y-0which is a no-op — still unaddressed.
-translate-y-0translates by zero pixels, so the chevrons won't be vertically centered despitetop-1/2. This should be-translate-y-1/2.🐛 Proposed fix (both chevron buttons)
- 'top-1/2 -left-16 -translate-y-0 absolute text-white', + 'top-1/2 -left-16 -translate-y-1/2 absolute text-white',- 'top-1/2 -right-16 -translate-y-0 absolute text-white', + 'top-1/2 -right-16 -translate-y-1/2 absolute text-white',
75-77:⚠️ Potential issue | 🟡 MinorConflicting
relativeandabsoluteon the same element — still unaddressed.Line 77 applies both
relativeandabsolute. The centering pattern (left-1/2 -translate-x-1/2 … absolute top-5) requires onlyabsolute. Removerelative.🐛 Proposed fix
- class="bg-(--p-toolbar-background)/60 text-white rounded-lg p-1 flex flex-row grow relative left-1/2 -translate-x-1/2 absolute top-5 -translate-y-1/2 z-10 w-70" + class="bg-(--p-toolbar-background)/60 text-white rounded-lg p-1 flex flex-row grow left-1/2 -translate-x-1/2 absolute top-5 -translate-y-1/2 z-10 w-70"
99-105:⚠️ Potential issue | 🟡 MinorNative
<button>still present — should be PrimeVue<Button>.The other action buttons were converted to PrimeVue
<Button>, but this "Open Gallery" button was missed. As per coding guidelines,**/*.vue: "Use TypeScript with Vue3 composition API and use PrimeVue for UI components."♻️ Proposed fix
- <button + <Button class="mt-4 sm:mt-2 inline-block px-6 py-3 sm:px-10 sm:py-4 lg:px-12 lg:py-4 text-2xl sm:text-sm lg:text-xs font-semibold tracking-wide sm:tracking-wider uppercase cursor-pointer hover:bg-white/10 w-fit border" :style="{ color: selectedColor, borderColor: selectedColor }" `@click`="scrollToPictures" - > - {{ $t("gallery.album.hero.open_gallery") }} - </button> + :label="$t('gallery.album.hero.open_gallery')" + />
125-125:⚠️ Potential issue | 🟡 MinorTypo:
UpdateAbumData→UpdateAlbumData— still unaddressed.The "l" is missing from "Album". The root cause is in
album-service.tswhere the type is defined; once fixed there, update this import accordingly.
163-174:⚠️ Potential issue | 🟡 MinorFallback palette values are not valid CSS colors — still unaddressed.
Values like
"blue-600"are Tailwind class names, not CSS color strings. When used in:style="{ color: selectedColor }", they won't render correctly. Use valid CSS colors (e.g.,"#2563eb") or fall back to"white".
255-275:⚠️ Potential issue | 🟠 Major
is_compact: falsehardcoded on save — still unaddressed.Line 268 forces
is_compacttofalseon every save, potentially overwriting the user's existing album display preference. Preserve the current value fromdata.🔧 Proposed fix
- is_compact: false, + is_compact: data.is_compact ?? false,
320-323:⚠️ Potential issue | 🟡 MinorMissing
.catch()onfetchLandingData()— previously fixed but appears regressed.This was flagged in a prior review and marked as addressed, but the current code still lacks error handling. An unhandled rejection here will leave
initdataasundefinedwith no feedback.🧯 Proposed fix
InitService.fetchLandingData().then((data) => { initdata.value = data.data; +}).catch((e) => { + console.error(e); });
🧹 Nitpick comments (4)
resources/js/components/gallery/albumModule/AlbumPanel.vue (2)
278-303: Minor inconsistency:header_photo_focusnot cleared onalbum.preFormattedDatain the toggle-off path.When toggling the header off,
header_photo_focusis nulled onmodelAlbum.preFormattedData(line 282) and onalbum.editable(line 292), but not onalbumStore.album.preFormattedData. The toggle-on path (line 302) does null it there. This is harmless in practice (the header URL is alsonullwhen toggled off, so the stale focus value is never consumed), but for consistency you could add the same reset in the toggle-off branch:Suggested diff
if (albumStore.album.preFormattedData) { if (isToggleOff) { albumStore.album.preFormattedData.url = null; + albumStore.album.preFormattedData.header_photo_focus = null; } else {
391-400:photoPanelis referenced in the closure before its declaration — safe at runtime but worth reordering.
scrollToPaginatorTop(line 392) capturesphotoPanel, which is declared on line 400, after thealbumCallbacksobject literal. Because the arrow function is never invoked during object construction, no TDZ error will occur at runtime. Still, movingphotoPanelabovealbumCallbacksmakes the dependency obvious and avoids confusing readers.Suggested reorder
+const photoPanel = ref<ComponentPublicInstance | null>(null); + const albumCallbacks = { ... }; -const photoPanel = ref<ComponentPublicInstance | null>(null);resources/js/components/gallery/albumModule/AlbumHeaderPanel.vue (2)
6-15: Usev-elseinstead of a secondv-ifwith the opposite condition.Lines 6 and 15 both use
v-iffor complementary boolean states ofis_album_enhanced_display_enabled. This evaluates both conditions independently on every render. Usev-if/v-elseto make the mutual exclusion explicit and avoid the redundant check.♻️ Proposed fix
<template v-if="!is_album_enhanced_display_enabled"> ... </template> - <template v-if="is_album_enhanced_display_enabled"> + <template v-else> ... </template>
301-304: Unchecked cast ofnewPosition— low risk but fragile.The
as keyof typeof POSITION_CLASSEScast has no runtime guard. Currently safe since all callers pass hardcoded valid keys, but a future caller passing an invalid string would silently produceundefinedclasses.🛡️ Defensive guard
function setPosition(newPosition: string) { + if (!(newPosition in POSITION_CLASSES)) { + console.warn(`Unknown position: ${newPosition}`); + return; + } position.value = newPosition as keyof typeof POSITION_CLASSES; emits("setPosition", newPosition); }
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/js/components/gallery/albumModule/AlbumHeaderPanel.vueresources/js/components/gallery/albumModule/AlbumPanel.vue
database/migrations/2026_02_16_165731_add_enhanced_album_display_configs.php
Outdated
Show resolved
Hide resolved
- Database & Config: Fixed typo album_enhanced_display_enable to album_enhanced_display_enabled in migrations, ConfigIntegrity.php, and InitConfig.php.
- Enums & TypeScript: Renamed
- AlbumTitleColor - options from color1..5 to colour_1..5 in AlbumTitleColor.php, Album.php, PreFormattedAlbumData.php, constants.ts, and lychee.d.ts.
- Resource Methods: Reverting change from $this->getHeader() to $this->getHeaderUrl() in all Album Resource classes (AlbumResource, HeadAlbumResource, SmartAlbumResource, TagAlbumResource).
- Frontend Naming: Renamed Vue model variables from albumCoverTitle* to albumHeaderTitle* in AlbumProperties.vue and constants.ts.
- Memory Leaks: Fixed HeaderFocusPicker.vue to destroy event listeners properly on unmount and correctly handle src changes using a watcher.
- Translations (Global): Changed the English source string for the title dropdowns from "Style" to "Header Title Style" and "Position" to "Header Title Position" in all 22 language gallery.php files.
- Translations (Bulgarian): Added missing album_enhanced_display_enabled and album_header_size translation strings to bg/all_settings.php.
- Strong Typing: Added array{x:string,y:string} PHPDoc types to HasHeaderFocusTrait.php and its interfaces.
- Fix issue with albums root rights not loaded when open gallery directly.
- Fix flagging photos in multiple views using "s" key.
…e constants Add ALBUM_TITLE_COLOR_ATTRIBUTE, ALBUM_TITLE_POSITION_ATTRIBUTE, and HEADER_PHOTO_FOCUS_ATTRIBUTE constants that were introduced in master via album header improvements (#4105) but missing from the feature branch. Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
3 participants
feat: Customizable album headers (avaliable for Pro licensed users)
Album Header Customization:
header_photo_focus columns on albums table
Summary by CodeRabbit
New Features
Localization
Tests
Chores