Add rating per user. #3899
Add rating per user. #3899
Conversation
📝 WalkthroughWalkthroughAdds photo star-rating: DB migrations and PhotoRating model; Statistics gains rating_sum/rating_count and rating_avg; new rating API/request/action/policy/resources; model relations and eager-loads; frontend components, composables, state/types, configs, translations, docs and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
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 |
Add PhotoRatingConcurrencyTest to verify: - Unique constraint prevents duplicate ratings - Same user rapid updates work correctly - Multiple users concurrent ratings maintain statistics consistency - Add/remove operations maintain data integrity Update database optimization tests to account for new photo_ratings table (table count increased from 34-35 to 35-36). Mark tasks I10 (Error Handling & Edge Cases) and I11 (Concurrency & Data Integrity Tests) as complete in feature 001 tasks document. Spec impact: Updates docs/specs/4-architecture/features/001-photo-star-rating/tasks.md to reflect completion of backend testing increments I10-I11. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add PhotoService.setRating() method to call the POST /Photo::setRating endpoint with type-safe rating values (0-5). Auto-generate TypeScript types from PHP resources using php artisan typescript:transform. This adds to PhotoResource: - current_user_rating: number | null And to PhotoStatisticsResource: - rating_count: number - rating_avg: number | null Document the typescript:transform command in coding-conventions.md to ensure developers regenerate types after modifying PHP resource classes. Mark task I7 (Frontend Service Layer) as complete in feature 001 tasks document. Spec impact: Updates docs/specs/3-reference/coding-conventions.md with TypeScript type generation workflow and docs/specs/4-architecture/features/001-photo-star-rating/tasks.md to reflect completion of frontend service layer increment I7. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Create PhotoRatingWidget Vue component with interactive 5-star rating interface: - Display average rating with half-star support when metrics enabled - Show user's current rating with hover preview - Remove rating button (0) to clear user's rating - Loading states with spinner during API calls - Toast notifications for success/error feedback - Error handling for 401/403/404 responses Integrate PhotoRatingWidget into PhotoDetails drawer, displaying below statistics section. Component updates photoStore.photo automatically after successful rating changes. Mark tasks I8 (PhotoRatingWidget Component) and I9 (Integrate into PhotoDetails) as complete in feature 001 tasks document. Spec impact: Updates docs/specs/4-architecture/features/001-photo-star-rating/tasks.md to reflect completion of frontend components I8-I9. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Create ThumbRatingOverlay component for compact star display on photo thumbnails: - Shows current user rating as filled stars - Hidden by default, visible on hover (opacity-0 group-hover:opacity-100) - Positioned top-right with backdrop blur Create PhotoRatingOverlay for full photo view: - Displays user's rating in top corner of photo view - Only shows when user has rated the photo - Integrates with existing Overlay component pattern Integrate overlays: - ThumbRatingOverlay added to PhotoThumb component - PhotoRatingOverlay added to PhotoPanel component Mark tasks I9a-I9d as complete in feature 001 tasks document. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update task tracking to reflect completion of photo rating overlay components and their integration. Spec impact: docs/specs/4-architecture/features/001-photo-star-rating/tasks.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add 6 configuration settings to control rating feature visibility: - ratings_enabled: master switch for rating feature - rating_show_avg_in_details: show average in photo details drawer - rating_show_avg_in_photo_view: show average in full photo view - rating_photo_view_mode: when to show rating overlay (always/hover/hidden) - rating_show_avg_in_album_view: show average on thumbnails - rating_album_view_mode: when to show rating on thumbnails (always/hover/hidden) Backend changes: - Migration adds 6 config rows with sensible defaults - PhotoController::rate() checks ratings_enabled config, throws ConfigurationException if disabled - InitConfig resource exposes all 6 settings to frontend Frontend changes: - LycheeState store includes 6 rating config properties - PhotoRatingWidget respects ratings_enabled and rating_show_avg_in_details settings - ThumbRatingOverlay respects ratings_enabled, rating_show_avg_in_album_view, and rating_album_view_mode - PhotoRatingOverlay respects ratings_enabled, rating_show_avg_in_photo_view, and rating_photo_view_mode - All components use actual config instead of placeholder computed properties All quality gates pass (PHPStan, php-cs-fixer, TypeScript, Prettier). Spec impact: docs/specs/4-architecture/features/001-photo-star-rating/tasks.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Create new VisibilityType enum (never/always/hover) and use it for rating configuration settings instead of string literals. Changes: - Add VisibilityType enum with NEVER, ALWAYS, HOVER cases - Update InitConfig to use VisibilityType for rating_photo_view_mode and rating_album_view_mode - Update migration to use "never" instead of "hidden" for consistency with existing ThumbOverlayVisibilityType - Update frontend LycheeState to use App.Enum.VisibilityType - Update Vue components to check for "never" instead of "hidden" - TypeScript types auto-generated with new VisibilityType enum All quality gates pass (PHPStan, php-cs-fixer, TypeScript, Prettier). Spec impact: docs/specs/4-architecture/features/001-photo-star-rating/tasks.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update rating config migration to extend BaseConfigMigration instead of implementing custom up/down methods. Changes: - Extend BaseConfigMigration instead of Migration - Use self::BOOL constant instead of '0|1' string literals - Add proper PHPDoc return type annotation for getConfigs() - Remove custom up() and down() methods (provided by BaseConfigMigration) All quality gates pass (PHPStan, php-cs-fixer). Spec impact: docs/specs/4-architecture/features/001-photo-star-rating/tasks.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update task tracking to reflect completion of config settings implementation for rating visibility. Completed deliverables: - 6 config settings using BaseConfigMigration - VisibilityType enum for type safety - PhotoController checks ratings_enabled config - Frontend components use actual config values - All quality gates passing Spec impact: docs/specs/4-architecture/features/001-photo-star-rating/tasks.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update database schema documentation and roadmap to reflect completion of photo rating feature. Changes to database-schema.md: - Add PhotoRating model documentation with fields, constraints, and relationships - Update Photo model to include PhotoRating and PhotoStatistics relationships - Update User model to include PhotoRating relationship Changes to roadmap.md: - Move Feature 001 from Active to Completed - Record completion date as 2025-12-27 - Add notes about feature capabilities Spec impact: docs/specs/4-architecture/features/001-photo-star-rating/tasks.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Mark final increments as complete in tasks.md. All 18 tasks delivered successfully. I12 (Documentation): - Updated database-schema.md with PhotoRating model - Updated roadmap.md to move feature from Active to Completed I13 (Final Quality Gate): - PHP: php-cs-fixer (0 changes), PHPStan (no errors) - Frontend: Prettier formatted, TypeScript validated - Tests: 21/21 passing (1090 assertions) Feature 001 Photo Star Rating is now complete and ready for production use. Spec impact: docs/specs/4-architecture/features/001-photo-star-rating/tasks.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add new increment I14 to address N+1 query problem in PhotoResource when loading rating collections. Problem: - PhotoResource fetches current_user_rating individually for each photo - Causes N+1 queries when loading albums/collections with many photos - Performance degrades linearly with photo count Solution approaches documented: 1. Eager load ratings relation in PhotoResource with loadMissing 2. Filter to current user in memory after loading 3. Alternative: Eager load with query constraint in controllers Estimated: 90 minutes Priority: High (performance issue) Spec impact: docs/specs/4-architecture/features/001-photo-star-rating/tasks.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/Unit/Actions/Db/OptimizeTablesTest.php (2)
26-30: Fix the incorrect docblock comment.The docblock describes checking "directories" and "license and copyright info," but this test actually verifies table optimization behavior. This appears to be copy-pasted from another test.
🔎 Proposed fix
/** - * Iterate over the directories and check if the files contain the correct license and copyright info.. + * Test that the OptimizeTables action returns the expected number of optimized table IDs. * * @return void */
31-36: Consider refactoring this brittle test to improve maintainability.The test checks for exact table counts
[3, 35, 36]without explaining what these magic numbers represent. This design is fragile:
- Every schema change requires updating this test
- The test doesn't verify WHICH tables are optimized, only HOW MANY
- The magic numbers lack context (what are tables 3, 35, and 36?)
Consider refactoring to:
- Verify the actual table names being optimized instead of counts
- Or at least add comments explaining what these numbers represent
- Or simplify to
self::assertGreaterThan(0, $output)if the exact count isn't critical🔎 Example refactor verifying table names
public function testOptimizeTables(): void { $optimize = new OptimizeTables(); - $output = count($optimize->do()); - self::assertTrue(in_array($output, [3, 35, 36], true), 'OptimizeTables should return either 3 or 35 or 36: ' . $output); + $tables = $optimize->do(); + self::assertNotEmpty($tables, 'OptimizeTables should return at least one table'); + // Or verify specific expected table names if possible + // self::assertContains('photos', $tables); }docs/specs/4-architecture/open-questions.md (1)
1-1143: Remove all resolved question entries per documentation standard; retain only active open questions.Coding guidelines specify: "remove each row as soon as it is resolved, ensuring the answer is captured first in the governing spec's normative sections." The instructions section explicitly states: "Resolve and remove: When answered, update the relevant spec sections (and create ADR if high-impact), then delete both the table row and Question Details entry."
Current file contains ~26 resolved questions (lines 13–179, strikethrough format) and ~22 archived entries (lines 213–296) that violate this removal requirement. Verification confirms that all decisions are properly captured in the feature spec (
docs/specs/4-architecture/features/001-photo-star-rating/spec.md):
- Q001-05, Q001-06 encoded in FR-001-01, FR-001-02, NFR-001-04
- Q001-13 encoded in NFR-001-02
- Q001-01, Q001-02, Q001-03, Q001-04 encoded in FR-001-09, FR-001-10
- Q001-11 encoded in FR-001-16
- Q001-19 encoded across telemetry sections
Since all decisions are durably recorded in the governing specs' normative sections (Requirements and NFRs), delete all strikethrough and archived entries. This file should contain only questions that remain unresolved.
docs/specs/3-reference/database-schema.md (1)
260-261: Update the "Last updated" date.The documentation has been modified to include the PhotoRating model, but the "Last updated" date still shows "December 22, 2025". Per coding guidelines, this should be updated to reflect the date of this update.
Update the date
--- -*Last updated: December 22, 2025* +*Last updated: December 27, 2025*
🧹 Nitpick comments (12)
docs/specs/4-architecture/open-questions.md (1)
441-441: Address three minor style issues flagged by static analysis.Three informal/weak word choices degrade writing quality:
- Line 441: "retry" is weak in formal spec context
- Line 656: "hard to read" could use stronger phrasing
- Line 922: "gonna" is too informal for technical documentation; use "going to"
🔎 Proposed fixes
- - May retry without fixing underlying issue + - May attempt retries without fixing underlying issue - - May be hard to read at small sizes + - May be difficult to read at small sizes - - YAGNI (You Aren't Gonna Need It) principle violation + - YAGNI (You Aren't Going to Need It) principle violationAlso applies to: 656-656, 922-922
resources/js/components/settings/ConfirmSave.vue (1)
13-13: Simplify redundant directional classes.Both
ltr:lg:w-3xsandrtl:lg:w-3xsapply the same width value. Since there's no directional difference, you can simplify this to justlg:w-3xs.🔎 Proposed simplification
- 'w-0 ltr:lg:w-3xs rtl:lg:w-3xs': !props.areAllSettingsEnabled, + 'w-0 lg:w-3xs': !props.areAllSettingsEnabled,database/migrations/2025_12_27_034137_create_photo_ratings_table.php (1)
25-25: Consider adding a CHECK constraint for the rating value.While application-level validation exists (in SetPhotoRatingRequest), a database CHECK constraint provides defense-in-depth to ensure rating values are always within the valid 1-5 range, preventing invalid data from direct database access or bugs.
🔎 Proposed enhancement
- $table->unsignedTinyInteger('rating')->comment('Rating value 1-5'); + $table->unsignedTinyInteger('rating')->comment('Rating value 1-5'); + + // Add CHECK constraint to enforce valid rating range + DB::statement('ALTER TABLE photo_ratings ADD CONSTRAINT check_rating_range CHECK (rating >= 1 AND rating <= 5)');Note: You'll also need to add the constraint drop in the
down()method:public function down(): void { Schema::table('photo_ratings', function (Blueprint $table) { $table->dropConstraintIfExists('check_rating_range'); }); Schema::dropIfExists('photo_ratings'); }Alternatively, use Laravel's native support if available in your version:
- $table->unsignedTinyInteger('rating')->comment('Rating value 1-5'); + $table->unsignedTinyInteger('rating')->comment('Rating value 1-5')->check('rating >= 1 AND rating <= 5');app/Actions/Photo/Timeline.php (1)
55-55: LGTM on rating addition. Consider removing duplicate 'statistics' entry.The 'rating' relationship is correctly added to the eager-loading list. However, 'statistics' appears twice in the array, which is redundant (though harmless).
Remove duplicate 'statistics' entry
- query: Photo::query()->with(['statistics', 'size_variants', 'statistics', 'palette', 'tags', 'rating']), + query: Photo::query()->with(['statistics', 'size_variants', 'palette', 'tags', 'rating']),lang/ru/gallery.php (1)
184-196: Consider localizing the rating strings to Russian.The rating translation block contains English strings instead of Russian translations. While functional, localizing these would improve the user experience for Russian-speaking users.
lang/zh_CN/gallery.php (1)
185-197: Consider localizing the rating strings to Chinese.The rating translation block contains English strings instead of Chinese translations. While functional, localizing these would improve the user experience for Chinese-speaking users.
app/Models/Photo.php (1)
246-257: Docstring mismatch: method returns HasOne, not "all ratings".The docstring on Line 247 says "Get all ratings for this photo" but this method returns a
HasOnerelationship filtered by the current user. Consider updating to clarify, e.g., "Get the rating for this photo by the current user (or anonymous rating if unauthenticated)."The implementation itself is correct and properly scopes to the authenticated user or anonymous ratings.
🔎 Suggested docstring fix
/** - * Get all ratings for this photo. + * Get the current user's rating for this photo, or the anonymous rating if unauthenticated. * * @return HasOne<PhotoRating,$this> */ public function rating(): HasOneresources/js/composables/album/photoActions.ts (1)
25-29: Consider logging when photo sync is skipped.The synchronization logic silently skips updating the photo in
photosStore.photosif the photo is not found (whenphotoIndex === -1). While this may be intentional for cases where the photo list hasn't been loaded yet, it could lead to subtle inconsistencies if the photo should be present but isn't found due to other issues.Optional: Add debug logging for skipped updates
// Update the photo in the album list (photosStore) to keep it in sync const photoIndex = photosStore.photos.findIndex((p) => p.id === photoStore.photo!.id); if (photoIndex !== -1) { photosStore.photos[photoIndex].is_starred = newStarValue; +} else { + // Photo not in current list - this is expected when viewing individual photos + console.debug('Photo not in current album list, skipping photosStore sync'); }app/Actions/Photo/Rating.php (1)
42-53: Consider documenting the race condition behavior for statistics creation.The
firstOrCreatecall at lines 42-53 could theoretically encounter a race condition if two concurrent requests attempt to create statistics for a photo without any. The DB unique constraint will handle this, causing one transaction to fail and retry via the outer try-catch.While the current implementation handles this correctly, consider adding a comment explaining this behavior:
// Ensure statistics record exists atomically (Q001-07) +// Note: firstOrCreate may encounter race conditions on initial creation, +// but the DB constraint ensures only one record is created. +// Failed transaction will be caught and retried via outer exception handler. $statistics = Statistics::firstOrCreate(This clarifies the expected behavior for future maintainers.
tests/Feature_v2/Photo/PhotoResourceRatingTest.php (1)
26-47: Test logic appears redundant.Line 29-33 creates a rating record, then Line 36-39 calls
Photo::setRatingwith the same rating value (4). This seems redundant - if you're testing that the resource includes the current user rating, you could either:
- Just call
Photo::setRatingwithout pre-creating the rating, OR- Fetch the photo resource directly after creating the rating (if there's a GET endpoint)
The current approach tests rating creation + update in one test, which might be better split or clarified.
app/Models/Statistics.php (1)
80-89: Consider returningnullinstead of0when there are no ratings.The accessor returns
0whenrating_countis zero, butPhotoStatisticsResourcedeclares?float $rating_avg = null, suggesting the intent may be to distinguish "no ratings" from "zero average". Returning0could cause the UI to display "0 stars" instead of "not rated".🔎 Proposed fix to return null for no ratings
protected function getRatingAvgAttribute(): float { - return $this->rating_count === 0 ? 0 : round($this->rating_sum / $this->rating_count, 2); + return $this->rating_count === 0 ? null : round($this->rating_sum / $this->rating_count, 2); }Note: If you apply this change, also update the PHPDoc on line 28 and the return type to
?float.tests/Feature_v2/Photo/SetPhotoRatingRequestTest.php (1)
44-53: The non-existent photo ID generation approach is acceptable but consider using a fixed test ID.The
random_bytesapproach works but introduces non-determinism. A fixed placeholder ID matching the expected format might make debugging easier if the test ever fails unexpectedly.🔎 Alternative with fixed test ID
public function testSetRatingWithNonExistentPhoto(): void { - // Generate a valid random ID format that doesn't exist in the database - $nonExistentId = strtr(base64_encode(random_bytes(18)), '+/', '-_'); + // Use a fixed valid-format ID that doesn't exist in the database + $nonExistentId = 'AAAAAAAAAAAAAAAAAAAAAAA_'; $response = $this->actingAs($this->userMayUpload1)->postJson('Photo::setRating', [ 'photo_id' => $nonExistentId, 'rating' => 5, ]); $this->assertNotFound($response); }
resources/js/components/gallery/photoModule/PhotoRatingOverlay.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
resources/js/views/gallery-panels/Search.vue (2)
276-287: Add error handling for the service call to prevent inconsistent UI state.The optimistic update to
albumStore.modelAlbum.cover_idoccurs without waiting for or handling the result ofPhotoService.setAsCover. If the API call fails, the UI will display an incorrect cover state with no user feedback or rollback mechanism.Per coding guidelines, use
.then()to handle the async result and implement rollback on failure.🔎 Proposed fix with error handling and rollback
setAsCover: () => { if (albumId.value === undefined) { return; } - PhotoService.setAsCover(selectedPhoto.value!.id, albumId.value); + const previousCoverId = albumStore.modelAlbum?.cover_id; + const newCoverId = previousCoverId === selectedPhoto.value!.id ? null : selectedPhoto.value!.id; + // Update the album's cover_id immediately to reflect the change (toggle behavior) if (albumStore.modelAlbum !== undefined) { - albumStore.modelAlbum.cover_id = albumStore.modelAlbum.cover_id === selectedPhoto.value!.id ? null : selectedPhoto.value!.id; + albumStore.modelAlbum.cover_id = newCoverId; } - AlbumService.clearCache(albumId.value); - // refresh(); + + PhotoService.setAsCover(selectedPhoto.value!.id, albumId.value).then( + () => { + AlbumService.clearCache(albumId.value); + }, + (error) => { + // Rollback on failure + if (albumStore.modelAlbum !== undefined) { + albumStore.modelAlbum.cover_id = previousCoverId; + } + toast.add({ severity: 'error', summary: trans('Error'), detail: trans('Failed to set cover'), life: 3000 }); + } + ); },
288-310: Add error handling for the service call to prevent inconsistent UI state.The optimistic updates to
albumStore.modelAlbum.header_idandalbumStore.album.preFormattedData.urloccur without waiting for or handling the result ofPhotoService.setAsHeader. If the API call fails, the UI will display an incorrect header state with no user feedback or rollback mechanism.Per coding guidelines, use
.then()to handle the async result and implement rollback on failure.🔎 Proposed fix with error handling and rollback
setAsHeader: () => { if (albumId.value === undefined) { return; } - PhotoService.setAsHeader(selectedPhoto.value!.id, albumId.value, false); + const previousHeaderId = albumStore.modelAlbum?.header_id; + const previousHeaderUrl = albumStore.album?.preFormattedData?.url; + // 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; } // Update the header image URL in the album's preFormattedData if (albumStore.album?.preFormattedData) { if (isToggleOff) { albumStore.album.preFormattedData.url = null; } else { // Use medium or small variant for the header image const headerUrl = selectedPhoto.value!.size_variants.medium?.url ?? selectedPhoto.value!.size_variants.small?.url ?? null; albumStore.album.preFormattedData.url = headerUrl; } } - AlbumService.clearCache(albumId.value); - // refresh(); + + PhotoService.setAsHeader(selectedPhoto.value!.id, albumId.value, false).then( + () => { + AlbumService.clearCache(albumId.value); + }, + (error) => { + // Rollback on failure + if (albumStore.modelAlbum !== undefined) { + albumStore.modelAlbum.header_id = previousHeaderId; + } + if (albumStore.album?.preFormattedData) { + albumStore.album.preFormattedData.url = previousHeaderUrl; + } + toast.add({ severity: 'error', summary: trans('Error'), detail: trans('Failed to set header'), life: 3000 }); + } + ); },resources/js/components/gallery/albumModule/AlbumPanel.vue (1)
310-322: Critical: Async operation not properly handled.The
PhotoService.setAsCovercall is asynchronous but the code immediately updatesalbumStore.modelAlbum.cover_idwithout waiting for the server response. If the service call fails, the UI will display incorrect state. Additionally, therefreshemit should only occur after the operation succeeds.🔎 Proposed fix using .then() chaining
setAsCover: () => { if (albumStore.album === undefined) return; if (selectedAlbum.value?.thumb?.id === undefined) return; - PhotoService.setAsCover(selectedAlbum.value!.thumb?.id, albumStore.album.id); - // Update the album's cover_id immediately to reflect the change (toggle behavior) - if (albumStore.modelAlbum !== undefined) { - albumStore.modelAlbum.cover_id = - albumStore.modelAlbum.cover_id === selectedAlbum.value!.thumb?.id ? null : selectedAlbum.value!.thumb?.id; - } - AlbumService.clearCache(albumStore.album.id); - emits("refresh"); + PhotoService.setAsCover(selectedAlbum.value!.thumb?.id, albumStore.album.id).then(() => { + // Update the album's cover_id after server confirms (toggle behavior) + if (albumStore.modelAlbum !== undefined) { + albumStore.modelAlbum.cover_id = + albumStore.modelAlbum.cover_id === selectedAlbum.value!.thumb?.id ? null : selectedAlbum.value!.thumb?.id; + } + AlbumService.clearCache(albumStore.album.id); + emits("refresh"); + }); },Based on coding guidelines: "Do not use await async calls in Vue3; use .then() instead"
♻️ Duplicate comments (2)
resources/js/components/gallery/albumModule/AlbumPanel.vue (2)
257-265: Critical: Async operation not properly handled.The
PhotoService.setAsCovercall is asynchronous but the code immediately updatesalbumStore.modelAlbum.cover_idwithout waiting for the server response. If the service call fails, the UI will display incorrect state. The new toggle behavior (setting tonullwhen already set) doesn't address the underlying async handling issue.🔎 Proposed fix using .then() chaining
setAsCover: () => { if (albumStore.album === undefined) return; - PhotoService.setAsCover(selectedPhoto.value!.id, albumStore.album.id); - // Update the album's cover_id immediately to reflect the change (toggle behavior) - if (albumStore.modelAlbum !== undefined) { - albumStore.modelAlbum.cover_id = albumStore.modelAlbum.cover_id === selectedPhoto.value!.id ? null : selectedPhoto.value!.id; - } - AlbumService.clearCache(albumStore.album.id); + PhotoService.setAsCover(selectedPhoto.value!.id, albumStore.album.id).then(() => { + // Update the album's cover_id after server confirms (toggle behavior) + if (albumStore.modelAlbum !== undefined) { + albumStore.modelAlbum.cover_id = albumStore.modelAlbum.cover_id === selectedPhoto.value!.id ? null : selectedPhoto.value!.id; + } + AlbumService.clearCache(albumStore.album.id); + }); },Based on coding guidelines: "Do not use await async calls in Vue3; use .then() instead"
266-285: Critical: Async operation not properly handled.The
PhotoService.setAsHeadercall is asynchronous but the code immediately updatesalbumStore.modelAlbum.header_idandalbumStore.album.preFormattedData.urlwithout waiting for the server response. If the service call fails, the UI will display incorrect state for both the header ID and URL. The new toggle behavior and URL updates don't address the underlying async handling issue.🔎 Proposed fix using .then() chaining
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; - } - // Update the header image URL in the album's preFormattedData - if (albumStore.album.preFormattedData) { - if (isToggleOff) { - albumStore.album.preFormattedData.url = null; - } else { - // Use medium or small variant for the header image - const headerUrl = selectedPhoto.value!.size_variants.medium?.url ?? selectedPhoto.value!.size_variants.small?.url ?? null; - albumStore.album.preFormattedData.url = headerUrl; - } - } - AlbumService.clearCache(albumStore.album.id); + const isToggleOff = albumStore.modelAlbum?.header_id === selectedPhoto.value!.id; + PhotoService.setAsHeader(selectedPhoto.value!.id, albumStore.album.id, false).then(() => { + // Update the album's header_id after server confirms (toggle behavior) + if (albumStore.modelAlbum !== undefined) { + albumStore.modelAlbum.header_id = isToggleOff ? null : selectedPhoto.value!.id; + } + // Update the header image URL in the album's preFormattedData + if (albumStore.album.preFormattedData) { + if (isToggleOff) { + albumStore.album.preFormattedData.url = null; + } else { + // Use medium or small variant for the header image + const headerUrl = selectedPhoto.value!.size_variants.medium?.url ?? selectedPhoto.value!.size_variants.small?.url ?? null; + albumStore.album.preFormattedData.url = headerUrl; + } + } + AlbumService.clearCache(albumStore.album.id); + }); },Based on coding guidelines: "Do not use await async calls in Vue3; use .then() instead"
🧹 Nitpick comments (1)
resources/js/views/gallery-panels/Search.vue (1)
281-284: Consider moving state updates to store actions.The direct mutation of
albumStore.modelAlbumandalbumStore.albumproperties from the component violates separation of concerns. Store state updates should be encapsulated in store actions to improve maintainability, testability, and make state changes easier to track.Consider creating actions like
albumStore.updateCoverIdOptimistically(photoId)andalbumStore.updateHeaderOptimistically(photoId, headerUrl)to centralize this logic.Also applies to: 293-307
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/js/components/gallery/albumModule/AlbumPanel.vueresources/js/composables/album/photoActions.tsresources/js/views/gallery-panels/Search.vue
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{vue,ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{vue,ts,js}: For Vue3/TypeScript frontend code, follow coding conventions defined in docs/specs/3-reference/coding-conventions.md for style, naming, and testing practices.
For frontend changes, run npm run check to ensure all frontend tests pass before committing.
Files:
resources/js/composables/album/photoActions.tsresources/js/components/gallery/albumModule/AlbumPanel.vueresources/js/views/gallery-panels/Search.vue
**/*.{vue,ts,js,css}
📄 CodeRabbit inference engine (AGENTS.md)
For frontend changes, run npm run format to apply frontend code formatting with Prettier before committing.
Files:
resources/js/composables/album/photoActions.tsresources/js/components/gallery/albumModule/AlbumPanel.vueresources/js/views/gallery-panels/Search.vue
**/*.vue
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.vue: Use TypeScript in composition API for Vue3 and use PrimeVue for UI components
Do not use await async calls in Vue3; use .then() instead
Do not use const function = () => {} syntax; use function functionName() {} instead in Vue3
In Vue3 components, the structure should be first, then <script lang="ts">, then <style>Files:
resources/js/components/gallery/albumModule/AlbumPanel.vueresources/js/views/gallery-panels/Search.vue🧠 Learnings (5)
📚 Learning: 2025-09-28T12:43:29.852Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3721 File: resources/js/views/gallery-panels/Album.vue:200-204 Timestamp: 2025-09-28T12:43:29.852Z Learning: The photoStore.load() method in resources/js/stores/PhotoState.ts already handles undefined photoId cases internally with a short circuit exit, so conditional checks before calling photoStore.load() are unnecessary.Applied to files:
resources/js/composables/album/photoActions.tsresources/js/components/gallery/albumModule/AlbumPanel.vueresources/js/views/gallery-panels/Search.vue📚 Learning: 2025-09-28T08:27:38.332Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3721 File: resources/js/composables/album/photoActions.ts:39-43 Timestamp: 2025-09-28T08:27:38.332Z Learning: The user ildyria (repository maintainer) prefers to skip error handling for the rotate photo functions (rotatePhotoCW and rotatePhotoCCW) in resources/js/composables/album/photoActions.ts.Applied to files:
resources/js/composables/album/photoActions.tsresources/js/components/gallery/albumModule/AlbumPanel.vue📚 Learning: 2025-09-28T08:27:31.767Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3721 File: resources/js/composables/album/photoActions.ts:27-31 Timestamp: 2025-09-28T08:27:31.767Z Learning: The maintainer ildyria has indicated that error handling for rotate functions in photoActions.ts should be ignored and not flagged as an issue.Applied to files:
resources/js/composables/album/photoActions.tsresources/js/components/gallery/albumModule/AlbumPanel.vue📚 Learning: 2025-12-22T14:11:17.217Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-12-22T14:11:17.217Z Learning: Applies to **/*.vue : Do not use await async calls in Vue3; use .then() insteadApplied to files:
resources/js/components/gallery/albumModule/AlbumPanel.vue📚 Learning: 2025-08-16T14:00:53.808Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3504 File: resources/js/composables/selections/selections.ts:145-147 Timestamp: 2025-08-16T14:00:53.808Z Learning: In the Lychee codebase, the maintainer ildyria has indicated that bounds checking for array access in selectables.albums.value[idx] within the albumClick function in resources/js/composables/selections/selections.ts is not necessary, suggesting there are adequate safeguards elsewhere in the code or the access pattern is guaranteed to be safe.Applied to files:
resources/js/components/gallery/albumModule/AlbumPanel.vue🧬 Code graph analysis (1)
resources/js/composables/album/photoActions.ts (3)
resources/js/stores/PhotoState.ts (1)
PhotoStore(14-14)resources/js/stores/PhotosState.ts (1)
usePhotosStore(7-33)resources/js/stores/AlbumState.ts (1)
useAlbumStore(10-150)⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: 3️⃣ Dockerfile Lint
- GitHub Check: 2️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.5 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.4 - mariadb -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
🔇 Additional comments (1)
resources/js/composables/album/photoActions.ts (1)
5-6: LGTM: Store imports and initialization.The addition of photosStore and albumStore enables state synchronization across multiple stores, which aligns with the rating-per-user feature requirements.
Also applies to: 12-13
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
resources/js/components/icons/StarRow.vue (1)
44-47: Consider adding prop validation for robustness.While the component handles fractional ratings correctly, adding runtime validation would prevent rendering issues if invalid data (negative values, NaN, values > 5) is passed to the component.
🔎 Optional: Add prop validation
const props = defineProps<{ rating: number; size: "small" | "medium" | "large"; }>(); + +// Defensive validation +const validatedRating = computed(() => { + if (isNaN(props.rating) || props.rating < 0) return 0; + if (props.rating > 5) return 5; + return props.rating; +});Then use
validatedRating.valueinstead ofprops.ratingin the template. However, this may be unnecessary if the component only receives data from trusted sources within the application.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/js/components/gallery/photoModule/PhotoRatingOverlay.vueresources/js/components/icons/StarRow.vuetests/Feature_v2/Photo/PhotoResourceRatingTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Feature_v2/Photo/PhotoResourceRatingTest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.vue
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.vue: Use TypeScript in composition API for Vue3 and use PrimeVue for UI components
Do not use await async calls in Vue3; use .then() instead
Do not use const function = () => {} syntax; use function functionName() {} instead in Vue3
In Vue3 components, the structure should be first, then <script lang="ts">, then <style>Files:
resources/js/components/icons/StarRow.vueresources/js/components/gallery/photoModule/PhotoRatingOverlay.vue**/*.{vue,ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{vue,ts,js}: For Vue3/TypeScript frontend code, follow coding conventions defined in docs/specs/3-reference/coding-conventions.md for style, naming, and testing practices.
For frontend changes, run npm run check to ensure all frontend tests pass before committing.Files:
resources/js/components/icons/StarRow.vueresources/js/components/gallery/photoModule/PhotoRatingOverlay.vue**/*.{vue,ts,js,css}
📄 CodeRabbit inference engine (AGENTS.md)
For frontend changes, run npm run format to apply frontend code formatting with Prettier before committing.
Files:
resources/js/components/icons/StarRow.vueresources/js/components/gallery/photoModule/PhotoRatingOverlay.vue⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: 3️⃣ Dockerfile Lint
- GitHub Check: 2️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.5 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
🔇 Additional comments (3)
resources/js/components/icons/StarRow.vue (1)
20-39: The edge case bug has been resolved correctly.The fractional rating logic now handles ratings with fractional parts ≥ 0.75 correctly by treating them as full stars. The fix matches the previous suggestion and the star rendering logic now works correctly across all rating ranges (0.0 to 5.0 with fractional support).
resources/js/components/gallery/photoModule/PhotoRatingOverlay.vue (2)
13-13: The CSS typo has been corrected.The transform class has been fixed from
-translate-1/2to the correct-translate-x-1/2, which properly centers the element horizontally.
13-13: Thetext-shadowclass is already properly defined inresources/sass/app.cssas a custom CSS utility (not a Tailwind utility), so no action is needed.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/Feature_v2/Photo/PhotoResourceRatingTest.php (1)
64-65: Use Configs facade without leading backslash.Lines 64-65 use
\Configs::set()with a leading backslash. For consistency with Laravel conventions and the rest of the codebase, import the facade at the top and use it without the backslash.🔎 Proposed fix
At the top of the file, add the import:
+use App\Models\Configs;Then update the method:
public function testPhotoResourceIncludesRatingStatisticsWhenMetricsEnabled(): void { // Enable metrics and set access to owner (userMayUpload1 owns photo1) - \Configs::set('metrics_enabled', '1'); - \Configs::set('metrics_access', 'owner'); + Configs::set('metrics_enabled', '1'); + Configs::set('metrics_access', 'owner');As per coding guidelines, avoid leading backslashes for class references when possible.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Http/Controllers/Gallery/PhotoController.phpapp/Http/Requests/Photo/SetPhotoRatingRequest.phptests/Feature_v2/Photo/PhotoResourceRatingTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Http/Controllers/Gallery/PhotoController.php
- app/Http/Requests/Photo/SetPhotoRatingRequest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
tests/Feature_v2/Photo/PhotoResourceRatingTest.php
tests/Feature_v2/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests in the tests/Feature_v2 directory should extend from BaseApiWithDataTest
Files:
tests/Feature_v2/Photo/PhotoResourceRatingTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
No need to mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Feature_v2/Photo/PhotoResourceRatingTest.php
🧠 Learnings (4)
📚 Learning: 2025-12-22T14:11:17.217Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T14:11:17.217Z
Learning: Applies to tests/Feature_v2/**/*.php : Tests in the tests/Feature_v2 directory should extend from BaseApiWithDataTest
Applied to files:
tests/Feature_v2/Photo/PhotoResourceRatingTest.php
📚 Learning: 2025-09-22T12:35:38.842Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3706
File: tests/Unit/Http/Controllers/ImportFromServerControllerTest.php:59-66
Timestamp: 2025-09-22T12:35:38.842Z
Learning: In the Lychee codebase, the Configs class extends Laravel's Model, making config values database-backed. Tests using the DatabaseTransactions trait automatically have config changes rolled back without requiring manual restoration in tearDown() methods.
Applied to files:
tests/Feature_v2/Photo/PhotoResourceRatingTest.php
📚 Learning: 2025-09-22T12:35:38.842Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3706
File: tests/Unit/Http/Controllers/ImportFromServerControllerTest.php:59-66
Timestamp: 2025-09-22T12:35:38.842Z
Learning: In the Lychee codebase, config values modified with Configs::set() are stored in the database and are automatically restored between tests when using the DatabaseTransactions trait, eliminating the need for manual restoration in tearDown methods.
Applied to files:
tests/Feature_v2/Photo/PhotoResourceRatingTest.php
📚 Learning: 2025-08-14T10:17:35.082Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3616
File: app/Actions/Album/PositionData.php:38-39
Timestamp: 2025-08-14T10:17:35.082Z
Learning: PositionDataResource uses toPhotoResources() method to convert photos to PhotoResource instances, and PhotoResource accesses the tags relationship ($photo->tags->pluck('name')->all()), making the 'tags' eager-loading in PositionData necessary to avoid N+1 queries.
Applied to files:
tests/Feature_v2/Photo/PhotoResourceRatingTest.php
🧬 Code graph analysis (1)
tests/Feature_v2/Photo/PhotoResourceRatingTest.php (5)
app/Models/Photo.php (1)
Photo(134-508)app/Models/PhotoRating.php (1)
PhotoRating(35-72)tests/Feature_v2/Base/BaseApiWithDataTest.php (1)
BaseApiWithDataTest(92-254)app/Services/Auth/SessionOrTokenGuard.php (1)
id(227-230)tests/Feature_v2/Base/BaseApiTest.php (1)
postJson(76-79)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: 3️⃣ Dockerfile Lint
- GitHub Check: 2️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.5 - mariadb
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
🔇 Additional comments (2)
tests/Feature_v2/Photo/PhotoResourceRatingTest.php (2)
49-59: LGTM!This test correctly verifies that posting a rating to a non-rated photo includes the new rating in the response.
112-133: The test expects 201 Created for an update operation, which violates REST conventions.When updating an existing rating (line 122-125), the expected status code should be
200 OKor204 No Content, not201 Created. The status code201 Createdshould only be returned when creating new resources.Either the endpoint should be modified to return the appropriate status code for updates, or if this design is intentional, it should be clearly documented with an explanatory comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/Feature_v2/Photo/PhotoRatingConcurrencyTest.php (3)
61-91: Use strict assertions for integer comparisons.Lines 78, 84, 89, and 90 use
assertEquals(), which performs loose comparison (==). Per coding guidelines requiring strict comparison (===), useassertSame()for integer values to prevent type coercion issues.🔎 Proposed fix
- $this->assertEquals(1, $ratingCount); + $this->assertSame(1, $ratingCount); // Verify final rating is the last one submitted $finalRating = PhotoRating::where('photo_id', $this->photo1->id) ->where('user_id', $this->userMayUpload1->id) ->value('rating'); - $this->assertEquals(1, $finalRating); + $this->assertSame(1, $finalRating); // Verify statistics are consistent (sum = 1, count = 1) $statistics = $this->photo1->statistics()->first(); $this->assertNotNull($statistics); - $this->assertEquals(1, $statistics->rating_sum); - $this->assertEquals(1, $statistics->rating_count); + $this->assertSame(1, $statistics->rating_sum); + $this->assertSame(1, $statistics->rating_count);As per coding guidelines, strict comparison is required.
97-130: Use strict assertions and remove redundant checks.Lines 126-127 use
assertEquals()instead ofassertSame()for integer comparisons. Additionally, lines 128-129 duplicate the assertions already made on lines 126-127 since$expectedSumis 12 and$expectedCountis 3.🔎 Proposed fix
$statistics = $this->photo1->statistics()->first(); $this->assertNotNull($statistics); - $this->assertEquals($expectedSum, $statistics->rating_sum); - $this->assertEquals($expectedCount, $statistics->rating_count); - $this->assertEquals(12, $statistics->rating_sum); // 5 + 4 + 3 - $this->assertEquals(3, $statistics->rating_count); + $this->assertSame($expectedSum, $statistics->rating_sum); + $this->assertSame($expectedCount, $statistics->rating_count);As per coding guidelines, use strict comparison and avoid code duplication.
135-160: Use strict assertions for integer comparisons.Lines 158-159 use
assertEquals()for integer comparisons. UseassertSame()to enforce strict comparison (===) as required by the coding guidelines.🔎 Proposed fix
$statistics = $this->photo1->statistics()->first(); $this->assertNotNull($statistics); - $this->assertEquals(4, $statistics->rating_sum); - $this->assertEquals(1, $statistics->rating_count); + $this->assertSame(4, $statistics->rating_sum); + $this->assertSame(1, $statistics->rating_count);As per coding guidelines, strict comparison is required.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Feature_v2/Photo/PhotoRatingConcurrencyTest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
tests/Feature_v2/Photo/PhotoRatingConcurrencyTest.php
tests/Feature_v2/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests in the tests/Feature_v2 directory should extend from BaseApiWithDataTest
Files:
tests/Feature_v2/Photo/PhotoRatingConcurrencyTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
No need to mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Feature_v2/Photo/PhotoRatingConcurrencyTest.php
🧠 Learnings (1)
📚 Learning: 2025-12-22T14:11:17.217Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T14:11:17.217Z
Learning: Applies to tests/Feature_v2/**/*.php : Tests in the tests/Feature_v2 directory should extend from BaseApiWithDataTest
Applied to files:
tests/Feature_v2/Photo/PhotoRatingConcurrencyTest.php
🧬 Code graph analysis (1)
tests/Feature_v2/Photo/PhotoRatingConcurrencyTest.php (5)
app/Exceptions/ModelDBException.php (1)
ModelDBException(33-56)app/Models/PhotoRating.php (1)
PhotoRating(35-72)tests/Feature_v2/Base/BaseApiWithDataTest.php (1)
BaseApiWithDataTest(92-254)app/Models/Photo.php (1)
ratings(241-244)tests/Traits/CatchFailures.php (1)
assertCreated(123-126)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: 2️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.5 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.