Conversation
…stions Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
…line editing to spec/plan/tasks
|
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 an admin Bulk Album Edit feature: backend action and DTO, controller endpoints (index/ids/patch/setOwner/delete) with requests/resources, frontend page/components and service, routes/menu, translations, docs, and feature tests; includes minor JS fixes and search/model signature updates. 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 |
…uests, resources, routes, tests Agent-Logs-Url: https://github.com/LycheeOrg/Lychee/sessions/d0cca004-8d62-4085-9349-2c4620ad4010 Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
…enu, translations, Vue component Agent-Logs-Url: https://github.com/LycheeOrg/Lychee/sessions/d0cca004-8d62-4085-9349-2c4620ad4010 Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
Agent-Logs-Url: https://github.com/LycheeOrg/Lychee/sessions/1295eb42-81ed-4f37-9e90-081164a54720 Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
Agent-Logs-Url: https://github.com/LycheeOrg/Lychee/sessions/9b5484cb-3ce1-4430-803b-ef6ee158d1d4 Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (12)
docs/specs/4-architecture/open-questions.md (1)
3002-3056: Resolved Q-034 entries look consistent.The four Q-034 entries follow the established format (Feature/Priority/Status/Opened/Resolved/Resolution/Spec Impact) used by adjacent resolved questions and capture the decisions that drive the implementation (TagAlbum exclusion, client-side depth via
_lft/_rgt, delete-confirmation modal, unfiltered "select all matching").Minor nit: the coding guideline asks that Markdown docs end with an
hr+*Last updated: [date]*. This file already has*Last updated: 2026-03-15*at line 2718 but subsequent additions (Q-023-01, Q-030-01..12, Q-033-, now Q-034-) have been appended after it. Consider moving that footer to the very end and refreshing the date, so it reflects the actual last update.As per coding guidelines: "At the bottom of documentation files, add an hr line followed by "Last updated: [date of the update]""
tests/Feature_v2/BulkAlbumEdit/SetOwnerTest.php (1)
88-89: Use snake_case for the local variable.
$subAlbumshould be$sub_albumto match the PHP variable naming convention.♻️ Proposed rename
- $subAlbum = Album::find($this->subAlbum1->id); - $this->assertSame($this->userMayUpload2->id, $subAlbum->owner_id); + $sub_album = Album::find($this->subAlbum1->id); + $this->assertSame($this->userMayUpload2->id, $sub_album->owner_id);As per coding guidelines, “Variable names should be in snake_case”.
app/Http/Requests/Admin/BulkAlbumEdit/PatchBulkAlbumRequest.php (1)
77-117: DRY: extract the optional-fields list into a class constant.The same 17-element
$optional_fieldsarray is duplicated inafter()andprocessValidatedValues(). If a new field is added later, it's easy to forget one of the two locations, leading to silently-ignored fields in the DTO. Extract to a private constant.♻️ Suggested refactor
class PatchBulkAlbumRequest extends BaseApiRequest { protected BulkAlbumPatchData $bulk_album_patch_data; + + private const OPTIONAL_FIELDS = [ + 'description', 'copyright', 'license', 'photo_layout', + 'photo_sorting_col', 'photo_sorting_order', + 'album_sorting_col', 'album_sorting_order', + 'album_thumb_aspect_ratio', 'album_timeline', 'photo_timeline', + 'is_nsfw', 'is_public', 'is_link_required', + 'grants_full_photo_access', 'grants_download', 'grants_upload', + ]; @@ public function after(): array { return [ function (\Illuminate\Validation\Validator $validator): void { - $optional_fields = [ - 'description', 'copyright', 'license', 'photo_layout', - 'photo_sorting_col', 'photo_sorting_order', - 'album_sorting_col', 'album_sorting_order', - 'album_thumb_aspect_ratio', 'album_timeline', 'photo_timeline', - 'is_nsfw', 'is_public', 'is_link_required', - 'grants_full_photo_access', 'grants_download', 'grants_upload', - ]; - $has_any = false; - foreach ($optional_fields as $field) { + foreach (self::OPTIONAL_FIELDS as $field) { if ($this->has($field)) { $has_any = true; break; } } @@ protected function processValidatedValues(array $values, array $files): void { - $optional_fields = [ - 'description', 'copyright', 'license', 'photo_layout', - 'photo_sorting_col', 'photo_sorting_order', - 'album_sorting_col', 'album_sorting_order', - 'album_thumb_aspect_ratio', 'album_timeline', 'photo_timeline', - 'is_nsfw', 'is_public', 'is_link_required', - 'grants_full_photo_access', 'grants_download', 'grants_upload', - ]; - $present = []; - foreach ($optional_fields as $field) { + foreach (self::OPTIONAL_FIELDS as $field) { if ($this->has($field)) { $present[] = $field; } }app/Actions/Admin/BulkEditAlbumsAction.php (1)
30-35: Prefer constructor injection overnew SetProtectionPolicy().Since
BulkEditAlbumsActionis instantiated inside the controller (new BulkEditAlbumsAction()inBulkAlbumController::patch) andSetProtectionPolicyhas no constructor args either, the current form works but forgoes Laravel's container for no gain. If you also convert the controller toapp(BulkEditAlbumsAction::class)or type-hint it in the method, you get testability (mock injection) for free.resources/js/services/bulk-album-edit-service.ts (1)
83-90:data: {}onaxios.getis unnecessary and inconsistent.
axios.get(url, { params, data: {} })is unusual — GET requests do not carry a body, anddatais ignored by most servers. If there's a specific reason (e.g. a proxy or request interceptor in the project requires it), please leave a comment; otherwise drop it to match thepatch/postcalls below.app/Http/Controllers/Admin/BulkAlbumController.php (1)
99-107: Re-constructedLengthAwarePaginatorloses path/query info and duplicates work.Creating a brand-new
LengthAwarePaginatordrops the request path, query string, and page name that the original$paginatedhad set, so any downstream rendering ofnext_page_url/prev_page_urlis broken.setCollection()maps items in-place and preserves everything else.♻️ Suggested refactor
- $resource_collection = $paginated->getCollection()->map(fn ($row) => $this->rowToResource($row)); - $mapped = new \Illuminate\Pagination\LengthAwarePaginator( - $resource_collection, - $paginated->total(), - $paginated->perPage(), - $paginated->currentPage(), - ); - - return new PaginatedBulkAlbumResource($mapped); + $paginated->setCollection( + $paginated->getCollection()->map(fn ($row) => $this->rowToResource($row)) + ); + + return new PaginatedBulkAlbumResource($paginated);resources/js/views/BulkAlbumEdit.vue (3)
337-340:UsersService.count()runs eagerly at<script setup>execution, unhandled rejections.Moving this to
onMounted()keeps component initialization cheap and synchronous with the rest of the lifecycle; more importantly, add a.catch()so a transient 5xx doesn't trigger an unhandled promise rejection in the console.🛠️ Suggested fix
const numUsers = ref(0); -UsersService.count().then((data) => { - numUsers.value = data.data; -}); +onMounted(() => { + UsersService.count() + .then((data) => { + numUsers.value = data.data; + }) + .catch(() => { + // non-fatal: hide the "Set Owner" button by default + numUsers.value = 0; + }); +});
367-367:activeSortingSelecttemplate-ref pattern is fragile.The same
ref="activeSortingSelect"is declared on two Select elements (photo sorting at line 204, album sorting at line 244) that are alternated byv-if. Vue treats same-named refs inside av-foras an array, but here both<Select>are outside av-for(they sit inside each row's cell). The ref is typed as an array (ref<Array<{...}>>([])), and you then callactiveSortingSelect.value[0]?.show()— which assumes the currently-rendered Select lands at index 0. This happens to work when only one is visible, but it's easy to break when a row re-renders or both cells race during transition. Prefer separate refs per sorting mode.Also applies to: 501-509
129-131:selectedIds.includes(...)on every row is O(n) per render.With
selectedIdsas an array andisPageAllSelected/ row checkboxes calling.includes(), rendering 100+ rows is O(n·m). SwitchingselectedIdsto aSet<string>(or maintaining a companionSetcomputed once per change) keeps per-row lookup O(1) and materially improves responsiveness when "select all matching" dumps 1 000 IDs into the selection.Also applies to: 387-389, 437-454
tests/Feature_v2/BulkAlbumEdit/PatchTest.php (1)
109-112: Potential NPE iflicensefails to cast.
$album->license->valuewill throw if$album->licenseis null. Given the model castslicensetoLicenseTypeand the PATCH just set'CC0', this should always resolve to an enum, but consider a safer assertion such asassertSame(LicenseType::CC0, $album->license)to make the test fail with a clearer message if the cast changes in the future.app/DTO/BulkAlbumPatchData.php (1)
146-151:filter_var(FILTER_VALIDATE_BOOLEAN)is redundant after Laravel'boolean'validation.
PatchBulkAlbumRequestvalidates each boolean field with'boolean', which viavalidated()already yields a proper PHPbool(Laravel acceptstrue/false/1/0/"1"/"0"). Runningfilter_varagain adds nothing and — more importantly — silently coerces unexpected values tofalseinstead of surfacing bugs. A plain(bool) $values[$field]or direct assignment is sufficient and clearer.docs/specs/4-architecture/features/034-bulk-album-edit/spec.md (1)
51-51: FR-034-05 banner text mismatches the ASCII mock-up.FR-034-05 quotes the banner as
"⚠ Any modification made on this page is final. There is no confirmation step."while the page layout mock-up on line 106 says"⚠ Any modification made on this page is final. There is no confirmation step."— these do match, but Goal#7on line 50 states "except delete". Consider also noting the delete-confirmation carve-out in FR-034-05 so the requirement is self-consistent with Goal#7and FR-034-10.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ab4a0f5-e1a3-4b49-b898-3afbdded328b
📒 Files selected for processing (53)
app/Actions/Admin/BulkEditAlbumsAction.phpapp/DTO/BulkAlbumPatchData.phpapp/Http/Controllers/Admin/BulkAlbumController.phpapp/Http/Requests/Admin/BulkAlbumEdit/DeleteBulkAlbumRequest.phpapp/Http/Requests/Admin/BulkAlbumEdit/IdsBulkAlbumRequest.phpapp/Http/Requests/Admin/BulkAlbumEdit/IndexBulkAlbumRequest.phpapp/Http/Requests/Admin/BulkAlbumEdit/PatchBulkAlbumRequest.phpapp/Http/Requests/Admin/BulkAlbumEdit/SetOwnerBulkAlbumRequest.phpapp/Http/Resources/Admin/BulkAlbumIdsResource.phpapp/Http/Resources/Admin/BulkAlbumResource.phpapp/Http/Resources/Admin/PaginatedBulkAlbumResource.phpapp/Metadata/Cache/RouteCacheManager.phpdocs/specs/4-architecture/features/034-bulk-album-edit/plan.mddocs/specs/4-architecture/features/034-bulk-album-edit/spec.mddocs/specs/4-architecture/features/034-bulk-album-edit/tasks.mddocs/specs/4-architecture/open-questions.mddocs/specs/4-architecture/roadmap.mdlang/ar/bulk_album_edit.phplang/bg/bulk_album_edit.phplang/cz/bulk_album_edit.phplang/de/bulk_album_edit.phplang/el/bulk_album_edit.phplang/en/bulk_album_edit.phplang/es/bulk_album_edit.phplang/fa/bulk_album_edit.phplang/fr/bulk_album_edit.phplang/hu/bulk_album_edit.phplang/it/bulk_album_edit.phplang/ja/bulk_album_edit.phplang/nl/bulk_album_edit.phplang/no/bulk_album_edit.phplang/pl/bulk_album_edit.phplang/pt/bulk_album_edit.phplang/ru/bulk_album_edit.phplang/sk/bulk_album_edit.phplang/sv/bulk_album_edit.phplang/tr/bulk_album_edit.phplang/vi/bulk_album_edit.phplang/zh_CN/bulk_album_edit.phplang/zh_TW/bulk_album_edit.phpresources/js/components/forms/bulk-album-edit/BulkEditFieldsDialog.vueresources/js/components/forms/bulk-album-edit/BulkSetOwnerDialog.vueresources/js/composables/contextMenus/leftMenu.tsresources/js/router/routes.tsresources/js/services/bulk-album-edit-service.tsresources/js/views/BulkAlbumEdit.vueroutes/api_v2.phproutes/web_v2.phptests/Feature_v2/BulkAlbumEdit/DeleteTest.phptests/Feature_v2/BulkAlbumEdit/IdsTest.phptests/Feature_v2/BulkAlbumEdit/IndexTest.phptests/Feature_v2/BulkAlbumEdit/PatchTest.phptests/Feature_v2/BulkAlbumEdit/SetOwnerTest.php
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
resources/js/views/BulkAlbumEdit.vue (1)
424-433: Debounced search has no in-flight cancellation — results can arrive out of order.
onSearchInputonly debounces the trigger; if a slow response for query"foo"lands after a faster response for"foobar",albums.valueends up showing stale rows for the older query. Consider tracking a request token / aborting the previous axios call, or at least ignoring responses whosesearchno longer matchessearch.value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bba3f98-424f-4028-8328-b5b43381a5a9
📒 Files selected for processing (7)
app/DTO/BulkAlbumPatchData.phpapp/Http/Requests/Admin/BulkAlbumEdit/PatchBulkAlbumRequest.phpresources/js/composables/contextMenus/leftMenu.tsresources/js/services/bulk-album-edit-service.tsresources/js/services/renamer-service.tsresources/js/services/webhook-service.tsresources/js/views/BulkAlbumEdit.vue
✅ Files skipped from review due to trivial changes (2)
- resources/js/services/renamer-service.ts
- resources/js/services/webhook-service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- resources/js/composables/contextMenus/leftMenu.ts
- app/Http/Requests/Admin/BulkAlbumEdit/PatchBulkAlbumRequest.php
- resources/js/services/bulk-album-edit-service.ts
- app/DTO/BulkAlbumPatchData.php
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)
app/Actions/Search/Strategies/Album/AlbumFieldLikeStrategy.php (1)
34-46:⚠️ Potential issue | 🟡 MinorAdd
QueryBuildertype to the closure parameter.The
apply()method acceptsBuilder|QueryBuilder, but the closure at line 43 only type-hints the parameter asBuilder. This creates a type inconsistency: if aQueryBuilderis passed and execution reaches the else branch, the closure receives a type it doesn't declare, causing type-checking to fail.Note: The method signature also violates the interface contract (
AlbumSearchTokenStrategydeclaresBuilderonly). IfQueryBuildersupport is necessary, the interface should be updated; otherwise, consider reverting the method signature to match the interface.🐛 Proposed fix
- $query->where(function (Builder $q) use ($pattern): void { + $query->where(function (Builder|QueryBuilder $q) use ($pattern): void { $q->whereRaw("base_albums.title LIKE ? ESCAPE '!'", [$pattern]) ->orWhereRaw("base_albums.description LIKE ? ESCAPE '!'", [$pattern]); });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2eb0814f-8c17-42d1-ad00-cbc459a9ca2c
📒 Files selected for processing (10)
app/Actions/Search/Strategies/Album/AlbumFieldLikeStrategy.phpapp/Http/Controllers/Admin/BulkAlbumController.phpapp/Http/Requests/Admin/BulkAlbumEdit/SetOwnerBulkAlbumRequest.phpapp/Http/Resources/Admin/BulkAlbumResource.phpresources/js/components/forms/album/AlbumProperties.vueresources/js/components/forms/bulk-album-edit/BulkEditFieldsDialog.vueresources/js/components/settings/ConfigGroup.vueresources/js/components/settings/General.vueresources/js/config/constants.tsresources/js/views/BulkAlbumEdit.vue
✅ Files skipped from review due to trivial changes (1)
- app/Http/Resources/Admin/BulkAlbumResource.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Http/Requests/Admin/BulkAlbumEdit/SetOwnerBulkAlbumRequest.php
- resources/js/views/BulkAlbumEdit.vue
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/Http/Controllers/Admin/BulkAlbumController.php (1)
133-138:⚠️ Potential issue | 🟡 MinorReject requests when any requested album ID is not transferred.
whereIn()silently drops IDs that do not resolve toAlbumrows, so the endpoint can return success while transferring only a subset. Compare the loaded IDs against the requested IDs before callingTransfer.🛠️ Proposed fix
+use Illuminate\Validation\ValidationException;DB::transaction(function () use ($album_ids, $owner_id, $transfer): void { /** `@var` Album[] $albums */ - $albums = Album::query()->whereIn('id', $album_ids)->get()->all(); + $albums = Album::query()->whereIn('id', $album_ids)->get()->keyBy('id'); + $missing_album_ids = array_values(array_diff($album_ids, $albums->keys()->all())); + + if ($missing_album_ids !== []) { + throw ValidationException::withMessages([ + 'album_ids' => 'One or more selected albums could not be transferred.', + ]); + } + foreach ($albums as $album) { $transfer->do($album, $owner_id); } });
🧹 Nitpick comments (1)
app/Http/Controllers/Admin/BulkAlbumController.php (1)
52-64: Avoid lazy-loadingownerand reduce repeated permission lookups in resource mapping.The index query doesn't eagerly load
owner, causing N+1 lazy-loads whenrowToResource()accesses it. Additionally,public_permissions()is called five times but should be cached once per row.♻️ Proposed refactor
- $query = Album::query()->without( + $query = Album::query()->with('owner')->without( ['cover', 'cover.size_variants', 'min_privilege_cover', 'min_privilege_cover.size_variants', 'max_privilege_cover', 'max_privilege_cover.size_variants', 'thumb'] )->orderBy('albums._lft', 'asc');private function rowToResource(Album $row): BulkAlbumResource { + $public_permissions = $row->public_permissions(); + return new BulkAlbumResource( @@ - is_public: $row->public_permissions() !== null, - is_link_required: $row->public_permissions()?->is_link_required === true, - grants_full_photo_access: $row->public_permissions()?->grants_full_photo_access === true, - grants_download: $row->public_permissions()?->grants_download === true, - grants_upload: $row->public_permissions()?->grants_upload === true, + is_public: $public_permissions !== null, + is_link_required: $public_permissions?->is_link_required === true, + grants_full_photo_access: $public_permissions?->grants_full_photo_access === true, + grants_download: $public_permissions?->grants_download === true, + grants_upload: $public_permissions?->grants_upload === true,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efa47f9c-a057-42eb-bf3b-f1c1948238ca
📒 Files selected for processing (2)
app/Http/Controllers/Admin/BulkAlbumController.phpapp/Models/Album.php
✅ Files skipped from review due to trivial changes (1)
- app/Models/Album.php
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd80cbed-0d61-4822-9d74-8396cefdc064
📒 Files selected for processing (1)
app/Http/Controllers/Admin/BulkAlbumController.php
Summary by CodeRabbit
New Features
Localization
Documentation
Tests