Support titles as dates for albums #3896
Conversation
📝 WalkthroughWalkthroughA new public static helper method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Http/Resources/Models/Utils/TimelineData.php (1)
75-75: Consider stricter regex validation for month and day ranges.The current regex pattern allows any 2-digit values for month and day (e.g., "2023-99-99"), which will then be caught by Carbon when it throws exceptions. You could add early validation for obviously invalid values:
$pattern = '/^(\d{4})(?:-(0[1-9]|1[0-2]))?(?:-(0[1-9]|[12][0-9]|3[01]))?/';This would match:
- Month: 01-12
- Day: 01-31
However, full date validation (e.g., Feb 31) still requires Carbon, so this is primarily an optimization.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Http/Resources/Models/Utils/TimelineData.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/Resources/Models/Utils/TimelineData.php
🧬 Code graph analysis (1)
app/Http/Resources/Models/Utils/TimelineData.php (1)
resources/js/lychee.d.ts (1)
ColumnSortingType(23-32)
⏰ 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). (18)
- GitHub Check: 3️⃣ Dockerfile Lint
- GitHub Check: 2️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.5 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- 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 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
🔇 Additional comments (2)
app/Http/Resources/Models/Utils/TimelineData.php (2)
20-21: Good use of Safe library for regex operations.Using
Safe\preg_matchand catchingPcreExceptionprovides robust error handling for regex operations.
101-102: Integration looks correct, but depends on exception handling fix.The usage of
parseDateFromTitle()is well-integrated with proper null handling at lines 106-108. However, this assumes the critical exception handling issue inparseDateFromTitle()is resolved to prevent uncaught exceptions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/Http/Resources/Models/Utils/TimelineData.php (1)
19-19:InvalidFormatExceptionis not thrown byCarbon::createFromDate().
Carbon::createFromDate()does not throwInvalidFormatException. In default mode, it overflows invalid dates (e.g., month 13 → January next year, as documented by your tests). In strict mode, it would throwInvalidArgumentException.The import at line 19 and the catch clause at line 87 are dead code. Consider either:
- Removing
InvalidFormatExceptionif overflow behavior is intentional (as tests confirm)- Catching
InvalidArgumentExceptionif you want to handle strict mode scenarios🔎 Proposed fix if overflow behavior is intentional
-use Carbon\Exceptions\InvalidFormatException;- } catch (PcreException|InvalidFormatException $e) { + } catch (PcreException $e) { // fail silently. return null;Also applies to: 87-89
tests/Unit/Http/Resources/TimelineDataTest.php (1)
105-127: Consider if overflow behavior is the desired UX.The tests document that invalid month (13) and day (32) values overflow to valid dates. This means an album titled "2023-13-01 Oops" would appear under January 2024 rather than being excluded.
If excluding malformed dates is preferred, consider validating ranges before creating the Carbon object:
if ($month < 1 || $month > 12 || $day < 1 || $day > 31) { return null; }However, if the current overflow behavior is intentional (perhaps for flexibility), the tests correctly document this design choice.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Http/Resources/Models/Utils/TimelineData.phptests/Unit/Http/Resources/TimelineDataTest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
app/Http/Resources/Models/Utils/TimelineData.phptests/Unit/Http/Resources/TimelineDataTest.php
tests/Unit/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests in the tests/Unit directory should extend from AbstractTestCase
Files:
tests/Unit/Http/Resources/TimelineDataTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
No need to mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Unit/Http/Resources/TimelineDataTest.php
🧠 Learnings (3)
📚 Learning: 2025-08-14T10:17:35.082Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3616
File: app/Actions/Album/PositionData.php:38-39
Timestamp: 2025-08-14T10:17:35.082Z
Learning: PositionDataResource uses toPhotoResources() method to convert photos to PhotoResource instances, and PhotoResource accesses the tags relationship ($photo->tags->pluck('name')->all()), making the 'tags' eager-loading in PositionData necessary to avoid N+1 queries.
Applied to files:
app/Http/Resources/Models/Utils/TimelineData.php
📚 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 **/*Resource.php : Resource classes should extend from Spatie Data instead of JsonResource
Applied to files:
app/Http/Resources/Models/Utils/TimelineData.php
📚 Learning: 2025-09-21T20:07:11.825Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3702
File: app/Http/Resources/Shop/CatalogResource.php:11-16
Timestamp: 2025-09-21T20:07:11.825Z
Learning: Spatie\LaravelData\DataCollection does NOT extend Illuminate\Support\Collection - it only implements some collection methods. To use Collection type, you must explicitly collect with Collection::class as the second parameter.
Applied to files:
app/Http/Resources/Models/Utils/TimelineData.php
🧬 Code graph analysis (1)
tests/Unit/Http/Resources/TimelineDataTest.php (1)
app/Http/Resources/Models/Utils/TimelineData.php (1)
parseDateFromTitle(68-91)
⏰ 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). (17)
- GitHub Check: 3️⃣ Dockerfile Lint
- GitHub Check: 2️⃣ PHP dist / 8.5 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- 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 -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.4 - postgresql -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
🔇 Additional comments (4)
app/Http/Resources/Models/Utils/TimelineData.php (2)
68-91: Well-structured helper with clear documentation.The implementation correctly:
- Uses a regex anchored at the start to ensure date prefix matching
- Defaults month/day to 1 when not provided
- Uses
Safe\preg_matchfor proper exception handling- Returns null gracefully when pattern doesn't match
The PHPDoc and inline comments clearly explain the expected formats.
102-104: Clean integration with existing pattern.The new
TITLEcase properly delegates to the helper and maintains the existing null-handling flow at line 107-109.tests/Unit/Http/Resources/TimelineDataTest.php (2)
1-23: Well-structured test file following coding guidelines.The file correctly:
- Includes the SPDX license header
- Extends
AbstractTestCaseas required for Unit tests- Uses appropriate
@noinspectionannotations for test exceptions
26-186: Comprehensive test coverage documenting expected behavior.The tests thoroughly cover:
- Valid formats (full date, year+month, year only)
- Edge cases (empty string, leading zeros, whitespace, extra hyphens)
- Invalid inputs (no date, date in middle, short year, non-standard format)
- Boundary dates (leap year, historical, future)
- Overflow behavior for invalid month/day values
The explicit documentation of Carbon's overflow behavior in
testParseDateFromTitleWithInvalidMonthandtestParseDateFromTitleWithInvalidDayis valuable for maintainability.
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.