Conversation
📝 WalkthroughWalkthroughReplaces legacy Debian/Nginx/PHP-FPM base with a FrankenPHP multi-stage Docker build, adds docker orchestration and validation scripts, restructures docker-compose to an API-centric internal network with hardened services, removes legacy base artifacts, and adds CI jobs for Docker linting, scanning, build, and push. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks✅ Passed checks (1 passed)
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: 11
🧹 Nitpick comments (3)
resources/js/app.ts (1)
39-39: Consider lazy loading for locales.The current glob pattern eagerly bundles all locale JSON files, which increases the initial bundle size. For applications with many locales, consider lazy loading:
-const langs = import.meta.glob("../../lang/*.json"); +const langs = import.meta.glob("../../lang/*.json", { eager: false });This would load locales on-demand, reducing the initial bundle size while still supporting dynamic locale switching.
docker/scripts/validate-env.sh (1)
10-15: Consider refactoring to avoid eval for safer code.While the current implementation is functional and the injection risk is minimal (since variable names are hardcoded), using
evalis generally discouraged. Consider using a safer alternative.🔎 Proposed refactor to avoid eval
# Check required variables for var in $REQUIRED_VARS; do - if [ -z "$(eval echo \$$var)" ]; then + value=$(env | grep "^${var}=" | cut -d= -f2-) + if [ -z "$value" ]; then echo "❌ ERROR: Required environment variable $var is not set" exit 1 fi doneAlternatively, if you need to support bash:
+#!/bin/bash -#!/bin/sh set -e # Check required variables for var in $REQUIRED_VARS; do - if [ -z "$(eval echo \$$var)" ]; then + if [ -z "${!var}" ]; then echo "❌ ERROR: Required environment variable $var is not set" exit 1 fi doneDockerfile (1)
28-50: Consider running npm audit separately for security checks.While using
npm ci --no-auditspeeds up the build, it skips security vulnerability checks. Consider runningnpm auditas a separate CI/CD step rather than skipping it entirely.💡 Suggestion
Keep the current build command but add a separate audit step in your CI pipeline:
# In CI/CD pipeline before building npm audit --productionThis ensures security checks without slowing down the Docker build.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.dockerignore.editorconfig.gitignoreDockerfileapp/Http/Kernel.phpapp/Http/Middleware/SetLocale.phpapp/Providers/AppServiceProvider.phpcomposer.jsondocker-compose.yamldocker/base/Dockerfiledocker/base/nginx.confdocker/base/start.shdocker/dev/Dockerfiledocker/scripts/entrypoint.shdocker/scripts/validate-env.shpackage.jsonresources/js/app.tsresources/js/components/settings/General.vue
💤 Files with no reviewable changes (6)
- composer.json
- docker/base/nginx.conf
- app/Providers/AppServiceProvider.php
- docker/dev/Dockerfile
- docker/base/start.sh
- docker/base/Dockerfile
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/app.tsresources/js/components/settings/General.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/app.tsresources/js/components/settings/General.vue
**/*.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:
app/Http/Middleware/SetLocale.phpapp/Http/Kernel.php
**/*.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/settings/General.vue🧠 Learnings (13)
📓 Common learnings
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3641 File: lang/no/settings.php:9-9 Timestamp: 2025-08-22T06:11:18.329Z Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.📚 Learning: 2025-09-28T08:39:34.280Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3721 File: resources/js/stores/NsfwConsentedState.ts:3-5 Timestamp: 2025-09-28T08:39:34.280Z Learning: In Lychee codebase, all Pinia store files follow the pattern of declaring the type alias before the store definition (e.g., `export type StoreType = ReturnType<typeof useStore>; export const useStore = defineStore(...)`). This pattern compiles successfully with TypeScript 5.9 and vue-tsc.Applied to files:
resources/js/app.ts📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-22T14:12:18.082Z Learning: Applies to **/*.php : For PHP changes, run vendor/bin/php-cs-fixer fix to apply PHP code style fixes before committing.Applied to files:
.editorconfig.dockerignore📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-22T14:12:18.082Z Learning: Applies to **/*.php : Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.Applied to files:
.editorconfig📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-22T14:12:18.082Z Learning: Applies to **/*.php : For PHP changes, run make phpstan to verify PHPStan level 6 minimum; fix all errors before committing.Applied to files:
.editorconfig📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-22T14:12:18.082Z Learning: Applies to **/*.php : For PHP changes, run php artisan test to ensure all tests pass before committing.Applied to files:
.editorconfig.dockerignore📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-22T14:12:18.082Z Learning: Applies to **/*.php : 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).Applied to files:
.editorconfig.dockerignore📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-22T14:12:18.082Z Learning: Applies to **/*.php : 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.Applied to files:
.editorconfig📚 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 **/*.php : Apply the PSR-4 coding standard in PHPApplied to files:
.editorconfig📚 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 : Use TypeScript in composition API for Vue3 and use PrimeVue for UI componentsApplied to files:
package.json📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR Repo: LycheeOrg/Lychee PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-22T14:12:18.082Z Learning: Applies to **/*.{vue,ts,js,css} : For frontend changes, run npm run format to apply frontend code formatting with Prettier before committing.Applied to files:
package.json📚 Learning: 2025-11-26T21:50:26.687Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 0 File: :0-0 Timestamp: 2025-11-26T21:50:26.687Z Learning: In the LycheeOrg/Lychee repository, lang/* files should not be reviewed at all. These files should be completely excluded from code review.Applied to files:
.gitignore📚 Learning: 2025-12-19T21:01:40.134Z
Learnt from: ildyria Repo: LycheeOrg/Lychee PR: 3838 File: lang/pl/webshop.php:1-2 Timestamp: 2025-12-19T21:01:40.134Z Learning: In the LycheeOrg/Lychee repository, files in the lang/* directory do not require the license header that is normally required for PHP files.Applied to files:
.gitignore🧬 Code graph analysis (2)
app/Http/Middleware/SetLocale.php (1)
app/Repositories/ConfigManager.php (2)
ConfigManager(22-262)getValueAsString(80-83)app/Http/Kernel.php (1)
app/Http/Middleware/SetLocale.php (1)
SetLocale(14-29)🔇 Additional comments (15)
.gitignore (1)
61-62: LGTM!The additions appropriately extend gitignore patterns to support the FrankenPHP migration. The case-sensitive variant
lychee/*alongsideLychee/*ensures consistent behavior across platforms, and the FrankenPHP-related ignores (frankenphpandfrankenphp-worker.php) prevent build artifacts from being committed.Also applies to: 69-70
resources/js/components/settings/General.vue (2)
253-253: LGTM!The import of
loadLanguageAsyncis correctly added to support the new language loading flow.
321-321: RTL language list is correct as-is.The hardcoded list
["ar", "fa"]accurately covers all RTL languages in the currently supported locales (ar, cz, de, el, en, es, fa, fr, hu, it, ja, nl, no, pl, pt, ru, sk, sv, vi, zh_CN, zh_TW). The suggested languages (Hebrew, Urdu, Yiddish) are not part of the application's supported locales and therefore should not be added to the RTL list..editorconfig (2)
28-31: LGTM!The lang/**.php block configuration is appropriate and consistent with PHP formatting standards.
32-34: LGTM!The docker-compose.yaml editorconfig block is appropriate with standard 2-space YAML indentation.
package.json (1)
41-41: Moving @lychee-org/leaflet.photo to dependencies is appropriate.This Leaflet plugin is now correctly categorized as a runtime dependency.
.dockerignore (1)
1-60: LGTM!The .dockerignore file is comprehensive and well-structured. Key highlights:
- Correctly excludes development, CI/CD, and test files
- Preserves .env.example while excluding sensitive .env files
- Excludes build artifacts (node_modules, storage/logs, cache) appropriately
- Does not exclude vendor/, which is correct since it's copied from the composer stage
docker/scripts/entrypoint.sh (5)
1-7: LGTM!The script header and environment validation are properly configured with
set -efor fail-fast behavior.
10-32: Database readiness check is well-implemented.The retry logic with 30 attempts and 2-second intervals provides a reasonable 60-second timeout. The use of
nc -zfor port checking is appropriate.
34-36: LGTM!Clearing the bootstrap cache before migrations is good practice to avoid stale configuration issues.
38-40: LGTM!Running migrations with
--forceis appropriate for production environments to avoid interactive prompts.
42-52: LGTM!The Laravel optimization commands are properly ordered, and using
exec "$@"ensures the main process replaces the shell, which is essential for proper signal handling in containers.docker-compose.yaml (2)
236-243: LGTM!Volume mounts are properly configured with the
.envfile mounted as read-only for security. Thedepends_onwith health condition ensures proper startup order.
245-253: LGTM!The healthcheck configuration is well-tuned with appropriate intervals, retries, and start period. The 30-second start period accounts for database migrations during startup.
Dockerfile (1)
86-117: LGTM!The final production stage is well-structured with:
- Proper file ownership and permissions for Laravel directories
- Correct separation of ENTRYPOINT and CMD
- Appropriate port exposure for Octane
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
database/factories/PhotoFactory.php (1)
80-94: Remove unreachable return statement.Line 93 contains a
return $this;statement that is unreachable because line 92 already returns from the afterCreating callback. This is dead code.🔎 Proposed fix
public function with_palette(): Factory { return $this->afterCreating(function (Photo $photo) { Palette::factory()->with_colour_1(0xFF0000) ->with_colour_2(0x00FF00) ->with_colour_3(0x0000FF) ->with_colour_4(0xFFFF00) ->with_colour_5(0xFF00FF) ->create(['photo_id' => $photo->id]); $photo->fresh(); $photo->load('palette'); }); - - return $this; }
🧹 Nitpick comments (2)
app/Listeners/CacheListener.php (1)
28-28: LGTM with an optional performance consideration.The migration to
app(ConfigManager::class)is correct for Octane compatibility.Optional: Since
handle()is invoked frequently for cache events, consider injectingConfigManageras a constructor dependency instead of resolving it per event. This would be more efficient:🔎 Optional refactor for better performance
class CacheListener { + public function __construct( + private readonly ConfigManager $config_manager + ) { + } + /** * Handle the event. */ public function handle(CacheHit|CacheMissed|KeyForgotten|KeyWritten $event): void { - $config_manager = app(ConfigManager::class); - if (str_contains($event->key, 'lv:dev-lycheeOrg')) { return; } - if ($config_manager->getValueAsBool('cache_event_logging') === false) { + if ($this->config_manager->getValueAsBool('cache_event_logging') === false) { return; }app/Metadata/Geodecoder.php (1)
39-39: LGTM with an optional refactor suggestion.The migration to
app(ConfigManager::class)is correct for Octane compatibility.Optional: The three static methods each independently resolve
ConfigManager. SincedecodeLocation()calls bothgetGeocoderProvider()anddecodeLocation_core(), a single decode operation resolves the manager three times. Consider passing the manager as a parameter to reduce redundant container resolutions:🔎 Optional refactor to reduce redundant resolutions
public static function getGeocoderProvider(): ProviderCache { - $config_manager = app(ConfigManager::class); + return self::getGeocoderProviderWithConfig(app(ConfigManager::class)); +} + +private static function getGeocoderProviderWithConfig(ConfigManager $config_manager): ProviderCache +{ try { $stack = HandlerStack::create(); // ... rest of implementation } } public static function decodeLocation(?float $latitude, ?float $longitude): ?string { $config_manager = app(ConfigManager::class); // User does not want to decode location data if (!$config_manager->getValueAsBool('location_decoding')) { return null; } if ($latitude === null || $longitude === null) { return null; } - $cached_provider = Geodecoder::getGeocoderProvider(); + $cached_provider = Geodecoder::getGeocoderProviderWithConfig($config_manager); - return Geodecoder::decodeLocation_core($latitude, $longitude, $cached_provider); + return Geodecoder::decodeLocation_core($latitude, $longitude, $cached_provider, $config_manager); } -public static function decodeLocation_core(float $latitude, float $longitude, ProviderCache $cached_provider): ?string +public static function decodeLocation_core(float $latitude, float $longitude, ProviderCache $cached_provider, ConfigManager $config_manager): ?string { - $config_manager = app(ConfigManager::class); - $lang = $config_manager->getValueAsString('lang'); // ... rest of implementation }Also applies to: 68-68, 92-92
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
app/Http/Middleware/SetLocale.phpapp/Jobs/ExtractColoursJob.phpapp/Jobs/ExtractZip.phpapp/Listeners/CacheListener.phpapp/Listeners/MetricsListener.phpapp/Mail/PhotosAdded.phpapp/Metadata/Geodecoder.phpapp/Metadata/Json/ChangeLogsRequest.phpapp/Metadata/Json/CommitsRequest.phpapp/Metadata/Json/TagsRequest.phpapp/Metadata/Json/UpdateRequest.phpapp/Metadata/Versions/InstalledVersion.phpcomposer.jsondatabase/factories/AccessPermissionFactory.phpdatabase/factories/AlbumFactory.phpdatabase/factories/OrderFactory.phpdatabase/factories/OrderItemFactory.phpdatabase/factories/PaletteFactory.phpdatabase/factories/PhotoFactory.phpdatabase/factories/PurchasableFactory.phpdatabase/factories/PurchasablePriceFactory.phpdatabase/factories/RenamerRuleFactory.phpdatabase/factories/SizeVariantFactory.phpdatabase/factories/StatisticsFactory.phpdatabase/factories/TagAlbumFactory.phpdatabase/factories/TagFactory.phpdatabase/factories/UserFactory.phpdatabase/factories/UserGroupFactory.phpphpstan.neon
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Http/Middleware/SetLocale.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
database/factories/PurchasablePriceFactory.phpdatabase/factories/OrderFactory.phpdatabase/factories/TagAlbumFactory.phpdatabase/factories/PurchasableFactory.phpdatabase/factories/AlbumFactory.phpapp/Mail/PhotosAdded.phpapp/Metadata/Json/CommitsRequest.phpdatabase/factories/PaletteFactory.phpapp/Jobs/ExtractColoursJob.phpapp/Metadata/Json/ChangeLogsRequest.phpdatabase/factories/SizeVariantFactory.phpdatabase/factories/UserGroupFactory.phpapp/Jobs/ExtractZip.phpapp/Listeners/MetricsListener.phpdatabase/factories/RenamerRuleFactory.phpdatabase/factories/StatisticsFactory.phpdatabase/factories/UserFactory.phpapp/Metadata/Json/UpdateRequest.phpdatabase/factories/PhotoFactory.phpapp/Listeners/CacheListener.phpapp/Metadata/Versions/InstalledVersion.phpdatabase/factories/TagFactory.phpdatabase/factories/OrderItemFactory.phpapp/Metadata/Geodecoder.phpapp/Metadata/Json/TagsRequest.phpdatabase/factories/AccessPermissionFactory.php
**/*Request.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*Request.php: In Request classes, if a user is provided by the query, place it in $this->user2
In Request classes, reserve $this->user for the user making the request
Files:
app/Metadata/Json/CommitsRequest.phpapp/Metadata/Json/ChangeLogsRequest.phpapp/Metadata/Json/UpdateRequest.phpapp/Metadata/Json/TagsRequest.php
🧠 Learnings (4)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
📚 Learning: 2025-08-14T17:22:19.225Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3618
File: database/factories/TagAlbumFactory.php:53-59
Timestamp: 2025-08-14T17:22:19.225Z
Learning: In TagAlbumFactory (test factory), the user ildyria prefers to keep the simpler loop-based attach() approach over syncWithoutDetaching() optimization since it's only used in tests where N+1 queries and performance are not a concern.
Applied to files:
database/factories/TagAlbumFactory.php
📚 Learning: 2025-09-12T20:17:52.985Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/CheckoutService.php:216-218
Timestamp: 2025-09-12T20:17:52.985Z
Learning: In the User model (app/Models/User.php), the name attribute uses a getNameAttribute() accessor method that returns a string type. It returns 'Admin' if the username starts with '$2y$' (indicating a hash), otherwise returns the username. This guarantees that $user->name will never be null when $user is not null.
Applied to files:
database/factories/UserFactory.php
📚 Learning: 2025-09-12T20:17:52.985Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3683
File: app/Actions/Shop/CheckoutService.php:216-218
Timestamp: 2025-09-12T20:17:52.985Z
Learning: In the User model (app/Models/User.php), the name attribute uses a getNameAttribute() accessor method that returns a string type and falls back to the username if name is null. This guarantees that $user->name will never be null when $user is not null.
Applied to files:
database/factories/UserFactory.php
🧬 Code graph analysis (11)
app/Mail/PhotosAdded.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Metadata/Json/CommitsRequest.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Jobs/ExtractColoursJob.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Metadata/Json/ChangeLogsRequest.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Jobs/ExtractZip.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Listeners/MetricsListener.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Metadata/Json/UpdateRequest.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Listeners/CacheListener.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Metadata/Versions/InstalledVersion.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Metadata/Geodecoder.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
app/Metadata/Json/TagsRequest.php (1)
app/Repositories/ConfigManager.php (1)
ConfigManager(22-262)
⏰ 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️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 2️⃣ PHP tests / 8.5 - mariadb -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.5 - postgresql -- Install
- GitHub Check: 2️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.5 - sqlite -- Install
- GitHub Check: 2️⃣ PHP tests / 8.5 - mariadb -- Install
- GitHub Check: 2️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 2️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 2️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 2️⃣ PHP tests / 8.4 - mariadb -- Install
- GitHub Check: 2️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.5 - sqlite -- Unit
🔇 Additional comments (30)
phpstan.neon (1)
16-16: LGTM!Expanding PHPStan analysis to include database/factories aligns with the factory PHPDoc improvements across the codebase and will help catch type-related issues early.
database/factories/UserGroupFactory.php (1)
16-20: LGTM!The PHPDoc annotation correctly specifies the factory's model type as
class-string<UserGroup>, which will help PHPStan catch type-related issues now that database/factories is included in the analysis paths.app/Jobs/ExtractZip.php (1)
88-88: LGTM!The migration to
app(ConfigManager::class)is correct. The manager is resolved inhandle()and used immediately without being stored in job properties, which properly avoids serialization issues for queued jobs.app/Jobs/ExtractColoursJob.php (1)
79-79: LGTM!The migration to
app(ConfigManager::class)is correct and follows the established pattern. The manager is resolved inhandle()and not stored in job properties, properly avoiding serialization concerns.app/Metadata/Versions/InstalledVersion.php (1)
68-68: LGTM!The migration to
app(ConfigManager::class)is correct and follows the established DI pattern for Octane compatibility.app/Mail/PhotosAdded.php (1)
34-36: LGTM. The migration toapp(ConfigManager::class)correctly supports Octane compatibility. The constructor immediately extracts the needed string value and discards the manager instance, avoiding serialization issues.Note: While
ConfigManagermaintains an internal cache and is not registered as a singleton in production, the usage pattern here is safe because the instance is used only to retrieve a single value and is not stored, eliminating any risk of state leakage between Octane requests.app/Listeners/MetricsListener.php (1)
25-25: LGTM: Container resolution improves testability.Switching from direct instantiation to container resolution (
app(ConfigManager::class)) is a Laravel best practice that allows for better dependency injection, singleton management, and easier mocking in tests.composer.json (1)
123-135: LGTM: Proper separation of production and development autoload paths.Moving
Scripts\\,PHPStan\\, andDatabase\\Factories\\toautoload-devcorrectly restricts these namespaces to development and testing environments, keeping the production autoload lean and focused on the core application.database/factories/SizeVariantFactory.php (1)
28-28: LGTM: Improved type annotation for static analysis.The refined docblock
@var class-string<SizeVariant>provides better type safety for PHPStan and aligns with the actual usage ofSizeVariant::class.database/factories/TagFactory.php (1)
16-21: LGTM: Added type annotation for model property.The added docblock with
@var class-string<Tag>provides proper type information for static analysis and documents the factory's corresponding model.database/factories/TagAlbumFactory.php (2)
27-27: LGTM: Improved type annotation for model property.The refined docblock
@var class-string<TagAlbum>provides better type safety for static analysis.
50-50: LGTM: Corrected return type annotation.Good catch! The return type was incorrectly documented as
PhotoFactorybut the method actually returnsTagAlbumFactory(self). This fix improves documentation accuracy.app/Metadata/Json/TagsRequest.php (1)
22-22: LGTM: Container resolution follows Laravel best practices.Resolving
ConfigManagervia the service container instead of direct instantiation improves testability and aligns with the broader dependency injection pattern adopted across this PR.database/factories/StatisticsFactory.php (1)
22-22: LGTM: Improved type annotation for static analysis.The refined docblock
@var class-string<Statistics>provides better type safety and aligns with PHPStan analysis requirements.database/factories/AlbumFactory.php (1)
26-26: LGTM: Improved type annotation for model property.The refined docblock
@var class-string<Album>provides better type safety for static analysis and completes the systematic factory documentation improvements across the codebase.database/factories/OrderFactory.php (1)
45-45: LGTM! Enhanced type annotation for static analysis.The PHPDoc refinement from
@var stringto@var class-string<Order>improves type precision for PHPStan without affecting runtime behavior.app/Metadata/Json/ChangeLogsRequest.php (1)
22-22: LGTM! Container resolution follows Laravel DI best practices.Replacing direct instantiation with
app(ConfigManager::class)improves testability and aligns with the broader DI refactor across the codebase.app/Metadata/Json/UpdateRequest.php (1)
22-22: LGTM! Container resolution follows Laravel DI best practices.The switch to
app(ConfigManager::class)improves testability and maintains consistency with similar refactors in ChangeLogsRequest and CommitsRequest.database/factories/PurchasablePriceFactory.php (1)
25-25: LGTM! Enhanced type annotation for static analysis.The PHPDoc refinement to
@var class-string<PurchasablePrice>improves type precision for PHPStan without runtime impact.database/factories/UserFactory.php (1)
27-27: LGTM! Enhanced type annotation for static analysis.The PHPDoc refinement to
@var class-string<User>improves type precision for PHPStan without affecting runtime behavior.database/factories/AccessPermissionFactory.php (1)
26-26: LGTM! Enhanced type annotation for static analysis.The PHPDoc refinement to
@var class-string<AccessPermission>improves type precision for PHPStan without runtime impact.database/factories/PaletteFactory.php (1)
23-23: LGTM! Enhanced type annotation for static analysis.The PHPDoc refinement to
@var class-string<Palette>improves type precision for PHPStan without affecting runtime behavior.app/Metadata/Json/CommitsRequest.php (1)
22-22: LGTM! Container resolution follows Laravel DI best practices.The switch to
app(ConfigManager::class)improves testability and maintains consistency with the broader DI refactor pattern across UpdateRequest, ChangeLogsRequest, and other classes.database/factories/PurchasableFactory.php (1)
26-26: LGTM! Improved type safety.The PHPDoc refinement to
class-string<Purchasable>enhances static analysis and aligns with the factory pattern's type requirements.database/factories/OrderItemFactory.php (2)
29-29: LGTM! Improved type safety.The PHPDoc refinement to
class-string<OrderItem>enhances static analysis.
95-95: Excellent fix! Corrects invalid instanceof usage.The change from
instanceof stringtois_string()fixes a critical bug. Theinstanceofoperator cannot be used with scalar types likestringin PHP and would always evaluate to false, causing incorrect behavior.Also applies to: 118-118
database/factories/PhotoFactory.php (2)
32-32: LGTM! Improved type safety.The PHPDoc refinement to
class-string<Photo>enhances static analysis.
99-99: LGTM! Corrected return type annotations.The return type annotations now correctly reflect that these methods return
PhotoFactory(viaself), notarray<string,mixed>. This aligns with the fluent interface pattern.Also applies to: 115-115
database/factories/RenamerRuleFactory.php (2)
16-16: LGTM! Corrected model reference.The class docblock now correctly references
RenamerRuleinstead of the incorrectRenamer, aligning with the actual model property.
23-23: LGTM! Improved type safety.The PHPDoc refinement to
class-string<RenamerRule>enhances static analysis.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
app/Http/Middleware/SetLocale.php (1)
16-28: Return type added, but constructor injection and exception handling still need refinement.The
: mixedreturn type is a good improvement. However, two recommendations from the previous review remain:
Constructor injection over app() helper: For middleware, Laravel best practice is to inject dependencies via the constructor. This leverages the service container's singleton pattern and makes the dependency explicit.
Overly broad exception catch: Catching
\Throwableincludes fatal errors and should generally be reserved for top-level handlers. Catching\Exceptionis more appropriate here.Silent failure: The caught exception is never logged, making it difficult to debug when locale configuration fails.
🔎 Proposed refactor with constructor DI and improved error handling
+use Illuminate\Support\Facades\Log; + class SetLocale { + public function __construct( + private readonly ConfigManager $configManager + ) { + } + public function handle(Request $request, \Closure $next): mixed { try { - $config_manager = app(ConfigManager::class); - $locale = $config_manager->getValueAsString('lang'); - } catch (\Throwable $e) { + $locale = $this->configManager->getValueAsString('lang'); + } catch (\Exception $e) { + Log::warning('Failed to retrieve locale from config', ['error' => $e->getMessage()]); $locale = config('app.locale'); } app()->setLocale($locale); return $next($request); } }docker-compose.yaml (2)
3-16: Avoid using:latesttag for production deployments.The image tag
lychee-frankenphp:latestlacks version pinning, which can lead to unexpected behavior when the image is updated.
255-274: Pin Redis image version and make user ID configurable.The
redis:alpineimage tag doesn't specify a version, anduser: 1026:100is hardcoded which may cause permission issues on different host systems.Dockerfile (1)
58-83: Remove potentially redundantgdpackage from apk.The
gdpackage is installed viaapk(line 61) and again viainstall-php-extensions(line 71). Theinstall-php-extensionstool typically handles system library dependencies automatically, so theapk add gdmay be redundant.Additionally, there's inconsistent indentation in this block (tabs on lines 65-66, 73, 78-82 vs spaces elsewhere).
🔎 Suggested fix
RUN apk add --no-cache \ exiftool \ ffmpeg \ - gd \ grep \ imagemagick \ jpegoptim \ - netcat-openbsd \ - unzip \ + netcat-openbsd \ + unzip \ curl \ && install-php-extensions \ pdo_mysql \ pdo_pgsql \ gd \ zip \ - dom \ + dom \ bcmath \ sodium \ opcache \ pcntl \ - exif \ - imagick \ - intl \ - redis \ - tokenizer \ - && rm -rf /var/cache/apk/* + exif \ + imagick \ + intl \ + redis \ + tokenizer \ + && rm -rf /var/cache/apk/*
🧹 Nitpick comments (2)
docker-compose.yaml (2)
25-25: Consider makingAPP_FORCE_HTTPSconfigurable.This value is hardcoded to
"false", but users deploying behind a reverse proxy with TLS termination may need this set to"true".🔎 Suggested fix
- APP_FORCE_HTTPS: "false" + APP_FORCE_HTTPS: "${APP_FORCE_HTTPS:-false}"
283-287: Validate that required database passwords are set.The insecure default passwords were removed (improvement from previous review), but if
DB_ROOT_PASSWORDorDB_PASSWORDare not set in the environment, MariaDB will receive empty strings which may cause silent failures or security issues.Consider using the
:?syntax to fail fast with a clear error:🔎 Suggested fix
environment: - - MYSQL_ROOT_PASSWORD=${DB_ROOT_PASSWORD} + - MYSQL_ROOT_PASSWORD=${DB_ROOT_PASSWORD:?DB_ROOT_PASSWORD must be set} - MYSQL_DATABASE=${DB_DATABASE:-lychee} - MYSQL_USER=${DB_USERNAME:-lychee} - - MYSQL_PASSWORD=${DB_PASSWORD} + - MYSQL_PASSWORD=${DB_PASSWORD:?DB_PASSWORD must be set}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Dockerfileapp/Http/Middleware/SetLocale.phpdatabase/factories/OrderItemFactory.phpdocker-compose.yamlresources/js/app.tsresources/js/components/settings/General.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/js/components/settings/General.vue
- resources/js/app.ts
- database/factories/OrderItemFactory.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
app/Http/Middleware/SetLocale.php
🧠 Learnings (4)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles (as used in Lychee), you may use --ignore-platform-reqs in the composer build stage when generating vendor files, because this stage only affects vendor generation and the final production image is what enforces platform requirements. Ensure: (1) this flag is confined to the build stage and does not affect the production runtime image, (2) the final stage uses the correct platform constraints and installed extensions, and (3) the produced vendor files are compatible with the production image’s PHP/runtime platform. This pattern should be applied to all Dockerfiles that contain a build stage with composer, not just a single file.
Applied to files:
Dockerfile
📚 Learning: 2025-08-22T06:11:18.329Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Applied to files:
Dockerfile
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
Applied to files:
docker-compose.yaml
🧬 Code graph analysis (1)
app/Http/Middleware/SetLocale.php (1)
app/Repositories/ConfigManager.php (2)
ConfigManager(22-262)getValueAsString(80-83)
🔇 Additional comments (6)
docker-compose.yaml (2)
241-250: LGTM!The
depends_onwithservice_healthycondition ensures the database is ready before the API starts, and the healthcheck configuration with appropriate intervals and retries is well-configured.
307-313: LGTM!Named volumes with explicit names ensure data persistence and easy identification.
Dockerfile (4)
8-23: LGTM!Using
--ignore-platform-reqsin the composer build stage is acceptable since this stage only generates vendor files that are copied to the production image. Based on learnings, the platform requirements that matter are enforced in the final production stage.
28-50: LGTM!The Node.js build stage follows best practices: using LTS version,
npm cifor deterministic installs, and proper layer caching by copying package files before source files.
104-116: LGTM!Clean separation between entrypoint (for initialization logic) and CMD (for the server process). The Octane configuration with FrankenPHP binding to
0.0.0.0:8000aligns with the docker-compose port mappings.
88-102: LGTM!The application copy strategy is sound: copying the application code, then overlaying vendor and built assets from previous stages. The storage and cache directory setup with proper ownership and permissions follows Laravel best practices. The
.dockerignorefile is properly configured with essential exclusions (.env, .git, node_modules, tests, etc.) to keep the build context clean.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/CICD.yml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : For PHP changes, run vendor/bin/php-cs-fixer fix to apply PHP code style fixes before committing.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.{vue,ts,js} : For frontend changes, run npm run check to ensure all frontend tests pass before committing.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : 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.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : For PHP changes, run make phpstan to verify PHPStan level 6 minimum; fix all errors before committing.
Applied to files:
.github/workflows/CICD.yml
⏰ 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.4 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- 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 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.4 - postgresql -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
🔇 Additional comments (10)
.github/workflows/CICD.yml (10)
23-25: LGTM! Concurrency control properly configured.The centralized concurrency configuration with selective cancel-in-progress is well-designed. It will save CI resources by canceling redundant runs on feature branches while protecting master and tag builds.
147-171: LGTM! Dockerfile linting properly configured.The hadolint action with
failure-threshold: warningensures high-quality Dockerfiles by failing on warnings. The job dependencies on both PHP and JS checks ensure the codebase is validated before Dockerfile validation.
175-180: Docker builds restricted to specific actor and branches.The condition limits Docker image builds to the 'ildyria' user, master branch, and tags. This means contributor PRs won't build or test Docker images, which could hide Docker-specific issues until after merge.
Consider documenting this restriction and providing guidance for contributors who need to verify Docker builds locally.
203-219: LGTM! Docker tagging strategy is well-designed.The metadata configuration follows best practices:
latesttag only for release tags (prevents master from being tagged as latest)edgetag for master branch builds- SHA-based tags for traceability
- Branch and tag references for clarity
220-230: LGTM! Docker build configuration is appropriate.The build action correctly:
- Enables GitHub Actions caching for faster builds
- Sets production environment for NODE_ENV
- Uses computed tags and labels from metadata step
236-245: Trivy scan is informational only.The vulnerability scanner has
exit-code: '0', meaning critical/high vulnerabilities will be reported but won't fail the build. This allows releases to proceed even with unfixed security issues.Consider changing to
exit-code: '1'if you want to block releases with critical vulnerabilities, or document this as intentional to avoid blocking on unfixable base image CVEs.
246-254: LGTM! Artifact creation properly depends on frontend checks.Adding
check_jsto the dependency list ensures the frontend is validated before building the release artifact. This aligns with best practices for full quality gates.Based on learnings, this ensures both PHP and frontend changes are validated before creating distribution artifacts.
326-327: LGTM! Release job renumbering is consistent.The job number update from 5️⃣ to 4️⃣ aligns with the overall workflow stage reorganization while maintaining the correct dependency on
createArtifact.
51-53: The referenced reusable workflow file./.github/workflows/js_check.ymlexists and is properly configured. It correctly defines aworkflow_calltrigger and runs JS checks across multiple Node versions (20 and 22) with appropriate security settings.
231-235: SHA tag extraction pattern is correct.The jq pattern
select(test(":sha-"))correctly matches the format produced by docker/metadata-action v5.10.0 withtype=sha. Tags are output asghcr.io/repo:sha-<commit>, and the pattern correctly filters for the:sha-substring.Note: With the current configuration (
type=shaspecified once), only one SHA tag will be produced. If the configuration is modified to include multipletype=shaentries, only the last matching tag would be used. Consider adding error handling if robustness against configuration changes is desired.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/CICD.yml (1)
33-33: Nice improvement to workflow readability.The emoji-numbered stages clearly communicate the CI/CD pipeline progression and make it easy to understand job sequencing at a glance.
Also applies to: 60-60, 85-85, 111-111, 138-138, 146-146, 247-247, 327-327
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/CICD.yml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : For PHP changes, run vendor/bin/php-cs-fixer fix to apply PHP code style fixes before committing.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.{vue,ts,js} : For frontend changes, run npm run check to ensure all frontend tests pass before committing.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : 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.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : For PHP changes, run make phpstan to verify PHPStan level 6 minimum; fix all errors before committing.
Applied to files:
.github/workflows/CICD.yml
⏰ 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.4 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.5 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- 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 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- 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 - mariadb -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
🔇 Additional comments (5)
.github/workflows/CICD.yml (5)
27-29: LGTM! Concurrency configuration is well-designed.The concurrency group correctly preserves master and tag builds while canceling redundant runs on feature branches and PRs.
55-57: LGTM! JS checks integrated correctly.The reusable workflow pattern is appropriate and the job runs in parallel with PHP syntax checks for faster feedback.
151-188: LGTM! Docker validation is comprehensive.The job correctly lints the Dockerfile and scans for vulnerabilities before allowing builds. Running Trivy on a locally built image catches issues early. The configuration appropriately fails on critical/high severity issues while ignoring unfixed vulnerabilities.
Note: This runs on all PRs, which provides good validation but may increase CI time.
220-244: LGTM! Docker metadata and build configuration are correct.The tagging strategy properly marks master builds as
edgeand tag builds aslatest. GitHub Actions cache integration will speed up repeated builds. TheNODE_ENV=productionbuild arg is appropriate for production Docker images.
246-254: LGTM! Artifact job dependencies updated correctly.Adding
check_jsto the dependency list ensures frontend validation completes before artifact creation, aligning with the full quality gate approach for changes affecting both backend and frontend.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
app/Actions/Diagnostics/Pipes/Checks/IniSettingsCheck.php (2)
45-48: Enhanced diagnostic output looks good.The addition of contextual details (current value and recommendation) significantly improves the diagnostic user experience.
Optional: Remove unnecessary parentheses
Line 45 has an extra pair of parentheses around
'30M':- if (Helpers::convertSize(ini_get('upload_max_filesize')) < Helpers::convertSize(('30M'))) { + if (Helpers::convertSize(ini_get('upload_max_filesize')) < Helpers::convertSize('30M')) {
49-52: Enhanced diagnostic output looks good.The addition of contextual details (current value and recommendation) improves the diagnostic output.
Optional: Remove unnecessary parentheses
Line 49 has an extra pair of parentheses around
'100M':- if (Helpers::convertSize(ini_get('post_max_size')) < Helpers::convertSize(('100M'))) { + if (Helpers::convertSize(ini_get('post_max_size')) < Helpers::convertSize('100M')) {docker-compose.yaml (1)
289-291: Consider pinning Redis image version.The
redis:alpinetag doesn't specify a version, which can lead to unexpected behavior when Alpine or Redis updates. A previous review recommendedredis:7-alpinefor reproducibility.🔎 Recommended fix
lychee_cache: - image: redis:alpine + image: redis:7-alpinedocker/scripts/permissions-check.sh (1)
19-22: Consider error handling for required directories.Lines 19-20 will fail with
set -eif/app/storageor/app/bootstrap/cachedon't exist (e.g., during initial container setup), while lines 21-22 correctly suppress errors for optional paths. You may want consistent error handling or ensure these directories are created in the Dockerfile.docker/scripts/validate-env.sh (1)
10-16: Variable expansion approach is safe but could be cleaner.The
eval echo "\${$var:-}"pattern works safely here sinceREQUIRED_VARSis hardcoded. However, a cleaner POSIX-compatible approach would useprintenv:🔎 Alternative approach
for var in $REQUIRED_VARS; do - val=$(eval echo "\${$var:-}") + val=$(printenv "$var" 2>/dev/null || true) if [ -z "$val" ]; then
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/CICD.ymlDockerfileapp/Actions/Diagnostics/Pipes/Checks/DBSupportCheck.phpapp/Actions/Diagnostics/Pipes/Checks/IniSettingsCheck.phpapp/Actions/Diagnostics/Pipes/Infos/DockerVersionInfo.phpconfig/features.phpdocker-compose.yamldocker/scripts/create-admin-user.shdocker/scripts/entrypoint.shdocker/scripts/permissions-check.shdocker/scripts/validate-env.sh
💤 Files with no reviewable changes (1)
- app/Actions/Diagnostics/Pipes/Checks/DBSupportCheck.php
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
config/features.phpapp/Actions/Diagnostics/Pipes/Checks/IniSettingsCheck.phpapp/Actions/Diagnostics/Pipes/Infos/DockerVersionInfo.php
🧠 Learnings (7)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : For PHP changes, run vendor/bin/php-cs-fixer fix to apply PHP code style fixes before committing.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.{vue,ts,js} : For frontend changes, run npm run check to ensure all frontend tests pass before committing.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : 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.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: Applies to **/*.php : For PHP changes, run make phpstan to verify PHPStan level 6 minimum; fix all errors before committing.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
Applied to files:
docker-compose.yaml
🧬 Code graph analysis (1)
app/Actions/Diagnostics/Pipes/Checks/IniSettingsCheck.php (2)
app/DTO/DiagnosticData.php (2)
DiagnosticData(13-72)warn(54-57)app/Assets/Helpers.php (2)
Helpers(20-296)convertSize(149-167)
🪛 Shellcheck (0.11.0)
docker/scripts/create-admin-user.sh
[warning] 2-2: In POSIX sh, set option pipefail is undefined.
(SC3040)
docker/scripts/validate-env.sh
[warning] 2-2: In POSIX sh, set option pipefail is undefined.
(SC3040)
docker/scripts/permissions-check.sh
[warning] 2-2: In POSIX sh, set option pipefail is undefined.
(SC3040)
docker/scripts/entrypoint.sh
[warning] 2-2: In POSIX sh, set option pipefail is undefined.
(SC3040)
⏰ 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). (14)
- GitHub Check: 2️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 1️⃣ PHP tests / 8.4 - postgresql -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.4 - mariadb -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- 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 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Install
🔇 Additional comments (9)
app/Actions/Diagnostics/Pipes/Checks/IniSettingsCheck.php (1)
53-59: LGTM! Enhanced diagnostic details provide clear guidance.The addition of current value and dual recommendations (200 seconds or 0 for no limit) gives users actionable information to resolve the issue.
app/Actions/Diagnostics/Pipes/Infos/DockerVersionInfo.php (1)
78-87: LGTM!The new
isLycheeFrankenPHP()method follows the established pattern of the existing Docker detection methods. The marker file/frankenphp_targetprovides a clean way to identify the FrankenPHP-based image, and the match expression ordering is correct..github/workflows/CICD.yml (2)
189-194: Docker build job dependency chain verified.The
docker-buildjob correctly depends ondocker_check(line 196), ensuring the Dockerfile is linted and the image passes vulnerability scanning before being pushed to the registry. The conditional logic is now correct.
246-253: Job dependency graph is well-structured.The
createArtifactjob correctly includescheck_jsin its dependencies, ensuring frontend checks complete before artifact creation. The parallel execution ofcreateArtifact(artifact build) anddocker-build(Docker image push) after their respective gates is an efficient design.docker-compose.yaml (1)
337-341: Verify database passwords are provided via .env file.MariaDB requires
MYSQL_ROOT_PASSWORD(orMYSQL_ALLOW_EMPTY_PASSWORD=yes) to start. With the inline password variables commented out, ensure your.envfile providesDB_ROOT_PASSWORDandDB_PASSWORD, or the container will fail to initialize.docker/scripts/create-admin-user.sh (1)
25-30: Good security hygiene.The script appropriately clears sensitive variables after use and prefers reading passwords from files over environment variables. The password length validation adds an extra layer of protection.
docker/scripts/entrypoint.sh (2)
66-75: Laravel bootstrap sequence is correct.The optimization sequence properly clears bootstrap cache first, runs migrations with
--forcefor non-interactive mode, then caches configuration, routes, and views. This is the recommended order for production deployments.
16-21: No action needed. Thenccommand is explicitly installed vianetcat-openbsdin the Dockerfile (line 72) as part of the production image build, so the database readiness check will work as intended without requiring a fallback.Likely an incorrect or invalid review comment.
docker/scripts/validate-env.sh (1)
18-23: Good APP_KEY format validation.Validating that the APP_KEY matches Laravel's expected
base64:...format with sufficient length is a solid security check. The helpful error message with the generation command is a nice touch for developers.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
docker/scripts/create-admin-user.sh (1)
1-35: LGTM - Security-conscious admin user creation.The script handles sensitive data appropriately by preferring file-based secrets over environment variables, enforcing minimum password length, and clearing sensitive variables after use. The shellcheck disable directive on line 2 explicitly acknowledges the pipefail compatibility concern flagged in past reviews.
Dockerfile (1)
64-91: Remove duplicate gd extension installation.The
gdextension is installed via bothapk(line 68) andinstall-php-extensions(line 78), causing unnecessary build overhead.🔎 Proposed fix
RUN apk add --no-cache \ exiftool \ shadow \ ffmpeg \ - gd \ grep \ imagemagick \docker-compose.yaml (1)
289-289: Pin Redis image to a specific version.Using
redis:alpinewithout version pinning can lead to unexpected updates and breaking changes. This issue was flagged in previous reviews but remains unresolved.🔎 Recommended fix
- image: redis:alpine + image: redis:7-alpine # Pin to Redis 7.x
🧹 Nitpick comments (1)
docker/scripts/entrypoint.sh (1)
10-33: Consider adding defensive validation for DB_HOST and DB_PORT.While
validate-env.shlikely checks these variables, adding a defensive check here before using them in thenccommand would provide clearer error messages and fail-fast behavior.🔎 Proposed enhancement
# Wait for database to be ready if [ "${DB_CONNECTION:-}" = "mysql" ] || [ "${DB_CONNECTION:-}" = "pgsql" ]; then + if [ -z "${DB_HOST:-}" ] || [ -z "${DB_PORT:-}" ]; then + echo "❌ ERROR: DB_HOST and DB_PORT must be set for ${DB_CONNECTION}" + exit 1 + fi + echo "⏳ Waiting for database to be ready..."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfiledocker-compose.yamldocker/scripts/create-admin-user.shdocker/scripts/entrypoint.shdocker/scripts/permissions-check.shdocker/scripts/validate-env.shscripts/delete_legacy.sh
💤 Files with no reviewable changes (1)
- scripts/delete_legacy.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- docker/scripts/validate-env.sh
- docker/scripts/permissions-check.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles (as used in Lychee), you may use --ignore-platform-reqs in the composer build stage when generating vendor files, because this stage only affects vendor generation and the final production image is what enforces platform requirements. Ensure: (1) this flag is confined to the build stage and does not affect the production runtime image, (2) the final stage uses the correct platform constraints and installed extensions, and (3) the produced vendor files are compatible with the production image’s PHP/runtime platform. This pattern should be applied to all Dockerfiles that contain a build stage with composer, not just a single file.
Applied to files:
Dockerfile
📚 Learning: 2025-08-22T06:11:18.329Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Applied to files:
Dockerfile
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
Applied to files:
docker-compose.yaml
🔇 Additional comments (3)
Dockerfile (1)
128-141: LGTM - Proper script and entrypoint configuration.The startup scripts are correctly copied with appropriate permissions, and the entrypoint/CMD configuration properly orchestrates the container startup with Laravel Octane and FrankenPHP.
docker/scripts/entrypoint.sh (1)
35-59: LGTM - Robust UID/GID management with security validation.The PUID/PGID validation enforces safe ranges (preventing root and system IDs), and the conditional user modification appropriately handles environments where the
shadowpackage is unavailable.docker-compose.yaml (1)
336-341: LGTM - Secure credential handling.Database passwords are appropriately required from the
.envfile rather than having insecure defaults. The commented lines forMYSQL_ROOT_PASSWORDandMYSQL_PASSWORD(lines 337, 340) ensure credentials must be explicitly provided.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
docker/scripts/create-admin-user.sh (1)
1-3: Pipefail shell compatibility addressed.The shellcheck disable directive on line 2 acknowledges the POSIX sh incompatibility. This is an acceptable resolution as long as the target container includes a shell that supports
pipefail(busybox ash in Alpine does support it).docker/scripts/permissions-check.sh (1)
1-3: Pipefail shell compatibility addressed.Same as create-admin-user.sh—the shellcheck disable directive acknowledges the POSIX incompatibility and is acceptable.
docker/scripts/entrypoint.sh (1)
1-3: Pipefail shell compatibility addressed.Same resolution as the other scripts—acceptable with shellcheck disable directive.
Dockerfile (1)
64-91: Remove duplicate gd extension installation.The
gdextension is installed via bothapk(line 68) andinstall-php-extensions(line 78). Remove the apk version to avoid redundancy and reduce build time.🔎 Proposed fix
RUN apk add --no-cache \ exiftool \ shadow \ ffmpeg \ - gd \ grep \ imagemagick \ jpegoptim \docker-compose.yaml (2)
288-316: Pin Redis version and make user ID configurable.Line 289 uses
redis:alpinewithout a version pin, which can lead to unexpected updates. Line 306 hardcodesuser: 999:999with a comment claiming this is the redis user in Alpine, but this may not be consistent across all Alpine versions or Redis images.🔎 Proposed fix
lychee_cache: - image: redis:alpine + image: redis:7-alpine # Pin to specific Redis major version hostname: lychee_cache security_opt: - no-new-privileges:true cap_drop: - ALL cap_add: - SETGID - SETUID read_only: true tmpfs: - /tmp:noexec,nosuid,nodev,size=50m healthcheck: test: ["CMD-SHELL", "redis-cli ping || exit 1"] # Removed port exposure - only accessible within Docker network # ports: # - ${REDIS_PORT:-6379}:${REDIS_PORT:-6379} - user: 999:999 # redis user in Alpine + user: "${REDIS_USER_ID:-999}:${REDIS_GROUP_ID:-999}" # redis user in Alpine
46-48: Align PUID/PGID defaults with entrypoint.sh and www-data.Lines 47-48 default
PUIDandPGIDto 1000, butentrypoint.shdefaults them to 33 (matching www-data in Alpine). The docker-compose values override the entrypoint defaults, potentially causing permission mismatches since the application expects www-data (33:33). Consider aligning both to 33 for consistency, or document why 1000 is preferred for Docker Compose deployments.🔎 Proposed fix for consistency
environment: - PUID: "${PUID:-1000}" - PGID: "${PGID:-1000}" + PUID: "${PUID:-33}" + PGID: "${PGID:-33}"
🧹 Nitpick comments (4)
docker/scripts/entrypoint.sh (2)
35-47: Add numeric validation for PUID/PGID.Lines 40-47 perform numeric range comparisons on
PUIDandPGIDwithout first validating they contain numeric values. If a user provides non-numeric values, the script may fail with unclear errors.🔎 Proposed fix
echo "Validating and setting PUID/PGID" PUID=${PUID:-33} PGID=${PGID:-33} +# Validate PUID/PGID are numeric +if ! printf '%s' "$PUID" | grep -qE '^[0-9]+$'; then + echo "❌ ERROR: PUID must be numeric (got: $PUID)" + exit 1 +fi +if ! printf '%s' "$PGID" | grep -qE '^[0-9]+$'; then + echo "❌ ERROR: PGID must be numeric (got: $PGID)" + exit 1 +fi + # Validate PUID/PGID are within safe ranges (no root, within system limits) if [ "$PUID" -lt 33 ] || [ "$PUID" -gt 65534 ]; then
67-76: Laravel artisan commands protected by set -e.The artisan commands (migrate, config:cache, etc.) will cause the script to exit on failure due to
set -eon line 3. However, for improved error visibility, consider adding explicit error checks with descriptive messages.🔎 Example: explicit error handling
# Run database migrations echo "🔄 Running database migrations..." -php artisan migrate --force +if ! php artisan migrate --force; then + echo "❌ ERROR: Database migration failed" + exit 1 +fi.github/workflows/CICD.yml (1)
176-177: Pass NODE_ENV consistently in both docker jobs.Line 177 builds the Docker image without specifying
NODE_ENV, relying on the Dockerfile's default. For consistency with thedocker-buildjob (line 243-244) and explicitness, consider passing--build-arg NODE_ENV=productionhere as well.🔎 Proposed fix
- name: Build Docker image locally - run: docker build -t lychee:local . + run: docker build --build-arg NODE_ENV=production -t lychee:local .docker-compose.yaml (1)
318-358: Consider more specific MariaDB version pin.Line 319 uses
mariadb:10, which pins the major version but allows minor version updates. For maximum reproducibility, consider pinning to a specific minor version (e.g.,mariadb:10.11). The security hardening (capabilities, tmpfs, healthcheck) and password handling via .env are well-implemented.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.dockerignore.github/workflows/CICD.ymlDockerfiledocker-compose.yamldocker/base/Dockerfiledocker/base/nginx.confdocker/base/start.shdocker/dev/Dockerfiledocker/scripts/create-admin-user.shdocker/scripts/entrypoint.shdocker/scripts/permissions-check.shdocker/scripts/validate-env.sh
💤 Files with no reviewable changes (4)
- docker/base/nginx.conf
- docker/base/Dockerfile
- docker/base/start.sh
- docker/dev/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (2)
- .dockerignore
- docker/scripts/validate-env.sh
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
Applied to files:
docker-compose.yaml
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles (as used in Lychee), you may use --ignore-platform-reqs in the composer build stage when generating vendor files, because this stage only affects vendor generation and the final production image is what enforces platform requirements. Ensure: (1) this flag is confined to the build stage and does not affect the production runtime image, (2) the final stage uses the correct platform constraints and installed extensions, and (3) the produced vendor files are compatible with the production image’s PHP/runtime platform. This pattern should be applied to all Dockerfiles that contain a build stage with composer, not just a single file.
Applied to files:
Dockerfile
📚 Learning: 2025-08-22T06:11:18.329Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Applied to files:
Dockerfile
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
Applied to files:
.github/workflows/CICD.yml
🔇 Additional comments (4)
docker/scripts/permissions-check.sh (1)
20-23: Inconsistent error handling is intentional for optional directories.Lines 20-21 enforce ownership on required directories (
/app/storage,/app/bootstrap/cache) without error suppression, ensuring they exist. Lines 22-23 suppress errors for optional directories (/app/public/uploads,/app/public/dist) that may not exist initially. This pattern is correct given the directory requirements.Dockerfile (1)
105-125: LGTM: Good security and operational practices.The directory setup, placeholder file creation, use of
php.ini-production, and custom PHP settings (upload limits, execution time, security headers) all follow best practices for production Laravel deployments..github/workflows/CICD.yml (1)
189-245: LGTM: Docker build and push job is well-configured.The job correctly uses conditional push logic, authenticated registry login, metadata extraction for tags, and build cache optimization. The explicit
NODE_ENV=productionbuild arg ensures consistent production builds.docker-compose.yaml (1)
4-16: Clarify image versioning strategy for lychee_api.Line 5 uses
lychee-frankenphp:latest, which was previously flagged and marked as addressed. If this docker-compose.yaml is intended for local development, the:latesttag is acceptable. However, for production deployments, use explicit version tags. Consider documenting the intended use case or providing a production-specific compose file with pinned versions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Dockerfile (1)
64-91: Remove duplicate gd extension installation.The
gdextension is installed via bothapk(line 68) andinstall-php-extensions(line 78). This redundancy increases build time unnecessarily.🔎 Proposed fix
RUN apk add --no-cache \ exiftool \ shadow \ ffmpeg \ - gd \ grep \ imagemagick \ jpegoptim \ netcat-openbsd \ unzip \ curl \ && install-php-extensions \ pdo_mysql \ pdo_pgsql \ gd \ zip \ dom \ bcmath \ sodium \ opcache \ pcntl \ exif \ imagick \ intl \ redis \ tokenizer \ && rm -rf /var/cache/apk/* \ && apk del shadowdocker/scripts/create-admin-user.sh (2)
9-16: Validate file readability before reading password.The script should verify
ADMIN_PASSWORD_FILEis readable before attempting to read it to provide clearer error messages.🔎 Proposed fix
# Prefer reading from file (more secure than env var) if [ -n "${ADMIN_PASSWORD_FILE:-}" ] && [ -f "${ADMIN_PASSWORD_FILE}" ]; then - # Securely read password file (no command substitution vulnerabilities) - value=$(cat "${ADMIN_PASSWORD_FILE}") + if [ -r "${ADMIN_PASSWORD_FILE}" ]; then + value=$(cat "${ADMIN_PASSWORD_FILE}") + else + echo "❌ ERROR: ADMIN_PASSWORD_FILE exists but is not readable" + exit 1 + fi elif [ -n "${ADMIN_PASSWORD:-}" ]; then
26-27: Verify admin user creation succeeds.The
php artisan lychee:create_usercommand should have its exit code checked. If user creation fails, the script should exit with an error rather than silently continuing.🔎 Proposed fix
echo "🚀 Creating admin account for user: ${ADMIN_USER}" - php artisan lychee:create_user "${ADMIN_USER}" "${value}" + if ! php artisan lychee:create_user "${ADMIN_USER}" "${value}"; then + echo "❌ ERROR: Failed to create admin user" + exit 1 + fidocker/scripts/entrypoint.sh (1)
10-33: Add DB_PORT validation to validate-env.sh.Line 18 uses
${DB_PORT}without validation. Thevalidate-env.shscript validatesDB_HOST,DB_DATABASE, andDB_USERNAMEfor mysql/pgsql connections, but does not validateDB_PORT. WhenDB_PORTis unset, thenccommand will fail with an unclear error.The validation should be added to
docker/scripts/validate-env.shin the conditional block that checks database credentials for mysql/pgsql connections.
🧹 Nitpick comments (1)
docker-compose.yaml (1)
288-316: Pin Redis image version for reproducibility.The
redis:alpineimage tag doesn't specify a version, which can lead to unexpected updates. For production deployments, specify an explicit version tag.🔎 Recommended fix
lychee_cache: - image: redis:alpine + image: redis:7-alpine # Pin to specific Redis version hostname: lychee_cache security_opt:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.dockerignore.github/workflows/CICD.ymlDockerfiledocker-compose.yamldocker/base/Dockerfiledocker/base/nginx.confdocker/base/start.shdocker/dev/Dockerfiledocker/scripts/create-admin-user.shdocker/scripts/entrypoint.shdocker/scripts/permissions-check.shdocker/scripts/validate-env.sh
💤 Files with no reviewable changes (4)
- docker/base/start.sh
- docker/base/nginx.conf
- docker/dev/Dockerfile
- docker/base/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (2)
- .dockerignore
- docker/scripts/permissions-check.sh
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
Applied to files:
.github/workflows/CICD.yml
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
Applied to files:
docker-compose.yaml
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles (as used in Lychee), you may use --ignore-platform-reqs in the composer build stage when generating vendor files, because this stage only affects vendor generation and the final production image is what enforces platform requirements. Ensure: (1) this flag is confined to the build stage and does not affect the production runtime image, (2) the final stage uses the correct platform constraints and installed extensions, and (3) the produced vendor files are compatible with the production image’s PHP/runtime platform. This pattern should be applied to all Dockerfiles that contain a build stage with composer, not just a single file.
Applied to files:
Dockerfile
📚 Learning: 2025-08-22T06:11:18.329Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
Applied to files:
Dockerfile
⏰ 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). (2)
- GitHub Check: 1️⃣ PHP 8.4 - Code Style errors
- GitHub Check: 1️⃣ PHP 8.4 - License Check
🔇 Additional comments (2)
.github/workflows/CICD.yml (2)
151-188: LGTM! Well-structured Docker validation job.The docker_check job properly validates the Dockerfile with Hadolint, builds locally, and scans with Trivy. The dependency on phpstan and check_js ensures code quality before Docker validation.
189-245: LGTM! Docker build and push job is correctly configured.The job properly:
- Conditionally runs only on master/tags
- Uses security hardening
- Authenticates to GHCR
- Leverages build cache
- Passes necessary build arguments
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.