Skip to content

Release v0.1.21 — Phase 21: Cleanup & Retention#293

Merged
menvil merged 36 commits into
mainfrom
release/v0.1.21-phase21-cleanup-retention
Jun 5, 2026
Merged

Release v0.1.21 — Phase 21: Cleanup & Retention#293
menvil merged 36 commits into
mainfrom
release/v0.1.21-phase21-cleanup-retention

Conversation

@menvil

@menvil menvil commented Jun 4, 2026

Copy link
Copy Markdown
Owner

Summary

  • FileRetentionPolicy service calculates expires_at from plan limits with strict validation (rejects non-integers like '1.5', zero, and negative values)
  • CleanupExpiredFilesJob deletes expired physical files via Storage::disk('local'), marks records FileStatus::Expired only after confirmed deletion (or if already missing); failed deletes are logged and retried on the next run; runs hourly via scheduler
  • files:cleanup-expired artisan command — prints "queued." for async dispatch, "completed." for --sync; confirmed with Bus::fake() assertions
  • Expired downloads blocked: web returns 410 with custom error view; API returns standardised result_expired JSON with expired_at timestamp (asserted in tests)
  • UI shows expiration label via expirationMetaFor() — no domain logic in Blade; Download hidden for expired results (both time-based and FileStatus::Expired)

What's in this release (CONV-330 → CONV-346)

Task Description
CONV-330 FileRetentionPolicy service (app/Services/Files/)
CONV-331–334 Upload & result file expiration (already implemented in prior phase; tests confirmed)
CONV-335 isExpired() checks status === Expired OR expires_at past
CONV-336–340 CleanupExpiredFilesJob: skeleton → physical deletion → record marking
CONV-341 files:cleanup-expired artisan command
CONV-342 Hourly scheduler registration with withoutOverlapping()
CONV-343 Expired web download → 410 + custom blade view
CONV-344 Expired API download → 410 result_expired JSON with expired_at
CONV-345 expirationMetaFor() in component; canDownload() respects expiry
CONV-346 Full retention lifecycle smoke tests

Post-review fixes

Finding Fix
Command always printed "completed" even for async dispatch Two branches: "queued." for dispatch, "completed." for --sync
Job marked DB record expired even if Storage::delete() failed exists()delete() → mark expired only on success or when file already gone; log + skip on failure
(int)"1.5" silently truncated to 1, passing validation filter_var($raw, FILTER_VALIDATE_INT, min_range:1) before cast
isExpired() called in Blade for CSS class selection expirationMetaFor() returns {label, class}; template renders prepared values only
API test missing expired_at assertion Added assertJsonPath('error.details.expired_at', ...)
Command test had no dispatch/not-dispatched assertions Added Bus::fake() + assertDispatched / assertNotDispatched
Missing-file cleanup test had no status assertion Added expect($file->fresh()->status)->toBe(FileStatus::Expired)
No Livewire test for FileStatus::Expired with future expires_at Added status-driven expiration test
Carbon::setTestNow() leaked between tests Global afterEach(fn () => Carbon::setTestNow()) in Pest.php
Cleanup job used config('filesystems.default') instead of 'local' Hardcoded to Storage::disk('local')

Test plan

  • composer test — 649 tests, all pass (was 620 before phase)
  • composer lint — clean
  • npm run build — clean
  • php artisan schedule:listfiles:cleanup-expired runs hourly
  • Upload → expire time passes → cleanup → file deleted, record marked Expired
  • Expired result download returns 410 (web + API), expired_at in API payload
  • Active files not touched by cleanup
  • Failed physical deletion skipped and logged; retried on next run

Deployment note

Ensure cron is configured on the server:

* * * * * php artisan schedule:run

🤖 Generated with Claude Code

menvil and others added 30 commits June 3, 2026 22:22
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests already existed from CONV-195; marked with CONV-331 reference.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Already implemented in StoreUploadedFileAction via FileExpirationPolicy.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Already implemented in RecordConversionResultFileAction via FileExpirationPolicy.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
isExpired() now checks both FileStatus::Expired and expires_at.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Runs files:cleanup-expired hourly with withoutOverlapping().

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Custom 410 view added; expanded download tests with expired status guard.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Returns result_expired JSON error (410) with expired_at timestamp.
Expiry check moved before storage existence check.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
menvil and others added 4 commits June 4, 2026 11:56
canDownload() blocks expired results; expiration label shows in actions column.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR implements file retention with automatic expiration and cleanup across the system. Users' uploads and conversion results receive plan-based expiration timestamps; a scheduled hourly job deletes expired files and marks their records; API and web downloads return 410 when accessing expired results; and the UI displays expiration labels and disables download actions accordingly.

Changes

File Retention and Expiration Lifecycle

Layer / File(s) Summary
Retention Policy, Exceptions, and Model Updates
app/Services/Files/FileRetentionPolicy.php, app/Exceptions/Files/InvalidRetentionPolicyException.php, app/Models/FileRecord.php
FileRetentionPolicy service computes user plan-based expiration timestamps and throws InvalidRetentionPolicyException for invalid retention days; FileRecord::isExpired() now checks both expires_at timestamp and status === FileStatus::Expired.
Cleanup Job — Expire and Delete Files
app/Jobs/CleanupExpiredFilesJob.php
CleanupExpiredFilesJob iterates through expired file records in batches, deletes associated stored files from the local disk, and marks records with FileStatus::Expired.
Cleanup Command and Scheduler Registration
app/Console/Commands/CleanupExpiredFilesCommand.php, routes/console.php
CleanupExpiredFilesCommand Artisan command supports --sync for synchronous execution or default queue dispatch; registered to run hourly with overlapping prevention.
API Download Endpoint — Expiration Handling and 410 Response
app/Http/Controllers/Api/V1/ConversionController.php, resources/views/errors/410.blade.php
API download endpoint explicitly checks result file presence and expiration, returning 410 with expired_at timestamp in error details for expired files; includes dedicated 410 Blade error view with user-facing messaging.
Recent Conversions Table — Expiration Label Display
app/Livewire/RecentConversionsTable.php, resources/views/livewire/recent-conversions-table.blade.php
RecentConversionsTable adds resultExpirationLabel() method rendering "Expired" or "Expires …" text; canDownload() now requires non-expired result file; template renders expiration status with conditional red/muted styling.
Web Download Path — Expiration Handling
tests/Feature/ConversionDownloadTest.php
Web-based download tests verify HTTP 410 responses with "expired" text when result files expire via timestamp or status.
Integration Tests — Retention Lifecycle and Job Behavior
tests/Feature/Retention/FileRetentionLifecycleTest.php, tests/Feature/Jobs/ProcessConversionJobTest.php
End-to-end tests covering: uploads receive plan-based expiration, cleanup removes expired files and marks records, conversion history remains while downloads block at 410, job is idempotent, active files are left untouched, and ProcessConversionJob assigns expiration to result files.
Unit Tests and Test Infrastructure
tests/Unit/Services/FileRetentionPolicyTest.php, tests/Feature/Files/FileRecordTest.php, tests/Feature/Api/V1/ConversionDownloadEndpointTest.php, tests/Feature/Livewire/RecentConversionsTableTest.php, tests/Feature/Console/CleanupExpiredFilesCommandTest.php, tests/Unit/Jobs/CleanupExpiredFilesJobSkeletonTest.php, tests/Pest.php
Unit tests for FileRetentionPolicy plan-based calculation and exception handling; feature tests for FileRecord::isExpired(), cleanup command, API download expiration, and Livewire UI; global Pest hook resets Carbon test clock after each test.

Sequence Diagram

sequenceDiagram
  participant User
  participant Upload/Conversion
  participant FileRetentionPolicy
  participant FileRecord
  participant CleanupJob
  participant APIDownload
  
  User->>Upload/Conversion: Upload file or convert
  Upload/Conversion->>FileRetentionPolicy: Get expiration for user
  FileRetentionPolicy-->>FileRecord: Set expires_at + status=Analyzed
  
  Note over FileRecord: File stored with expiration timestamp
  
  User->>APIDownload: Request download before expiry
  APIDownload->>FileRecord: Check isExpired()
  alt Not expired
    FileRecord-->>APIDownload: Return false
    APIDownload-->>User: 200 File download
  end
  
  Note over FileRecord: Time passes until expires_at reached
  
  CleanupJob->>FileRecord: Find expired records
  FileRecord-->>CleanupJob: Records with expires_at <= now
  CleanupJob->>CleanupJob: Delete stored file from disk
  CleanupJob->>FileRecord: Update status = Expired
  
  User->>APIDownload: Request download after expiry
  APIDownload->>FileRecord: Check isExpired()
  alt Expired
    FileRecord-->>APIDownload: Return true
    APIDownload-->>User: 410 File Expired
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • menvil/FileConverter#273: Modifies the API download path in ConversionController::download for missing/expired result files and 404/410 error handling.
  • menvil/FileConverter#182: Updates plan-based retention day configuration and FileExpirationPolicy integration with FeatureAccessService.
  • menvil/FileConverter#76: Introduces FileRecord model and FileStatus enum that this PR extends with expiration status checks and cleanup logic.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title accurately reflects the main objective: implementing file retention policies and automatic cleanup mechanisms for expired files as part of a release phase.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch release/v0.1.21-phase21-cleanup-retention

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the release Triggers AI code review (CodeRabbit, Cubic) label Jun 4, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 21 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tests/Unit/Services/FileRetentionPolicyTest.php">

<violation number="1" location="tests/Unit/Services/FileRetentionPolicyTest.php:12">
P2: `Carbon::setTestNow()` is set in tests but never restored. This leaks frozen time state to other tests, causing flaky, order-dependent failures when other tests rely on `now()` returning real time.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread app/Jobs/CleanupExpiredFilesJob.php Outdated
use Carbon\Carbon;

it('calculates expiration date from user retention limit', function () {
Carbon::setTestNow('2026-06-01 10:00:00');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Carbon::setTestNow() is set in tests but never restored. This leaks frozen time state to other tests, causing flaky, order-dependent failures when other tests rely on now() returning real time.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/Unit/Services/FileRetentionPolicyTest.php, line 12:

<comment>`Carbon::setTestNow()` is set in tests but never restored. This leaks frozen time state to other tests, causing flaky, order-dependent failures when other tests rely on `now()` returning real time.</comment>

<file context>
@@ -0,0 +1,59 @@
+use Carbon\Carbon;
+
+it('calculates expiration date from user retention limit', function () {
+    Carbon::setTestNow('2026-06-01 10:00:00');
+
+    $user = User::make(['plan' => Plan::Free]); // free plan: 1 retention day
</file context>

- CleanupExpiredFilesJob: use Storage::disk('local') explicitly instead of
  config('filesystems.default') — matches how all other file operations work
- Pest.php: add global afterEach(Carbon::setTestNow()) so frozen time never
  leaks across tests regardless of test order

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@menvil

menvil commented Jun 4, 2026

Copy link
Copy Markdown
Owner Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/Http/Controllers/Api/V1/ConversionController.php (1)

70-117: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Extract download eligibility/expiry decision flow into an Action.

The controller now owns a full retention/missing/expired decision tree and response shaping. Move this policy/decision logic into app/Actions and keep the controller as orchestration-only (authorize → invoke action → map response).

As per coding guidelines: **/{Http/Controllers/**/*.php,app/Livewire/**/*.php}: Controllers and Livewire components must stay thin - business logic goes into app/Actions.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Http/Controllers/Api/V1/ConversionController.php` around lines 70 - 117,
Move the download eligibility and expiry decision tree out of
ConversionController::download into a new Action (e.g.,
app/Actions/DetermineDownloadEligibility or DownloadEligibilityAction) that
accepts the authenticated user and a ConversionJob (or conversionId) and returns
a structured result/enum (allowed, not_ready, missing_result,
expired_with_timestamp, missing_file) plus any metadata (expires_at,
stored_path). In the Action implement the existing checks: ConversionJob
existence, ownership verification (you can call OwnershipGuard from the Action
or accept the pre-validated user and conversion), status !==
ConversionStatus::Completed, null resultFile, resultFile->isExpired() and
Storage::disk('local')->exists($resultFile->stored_path), and map each outcome
to a clear payload. Update ConversionController::download to be
orchestration-only: call the Action, then translate the Action's outcome into
the appropriate JSON response codes/messages (422, 404, 410, etc.) and return
the file stream when allowed. Ensure the Action and controller use the same
unique symbols (ConversionJob, ConversionStatus, OwnershipGuard,
resultFile->isExpired, Storage::disk) so locating logic is straightforward.
🧹 Nitpick comments (4)
tests/Feature/Livewire/RecentConversionsTableTest.php (1)

381-399: ⚡ Quick win

Add a status-based expired UI case.

This validates expired-by-time, but it misses expired-by-status (FileStatus::Expired with future expires_at), which is part of the same isExpired() contract.

Suggested additional test
+it('does not show download action when result status is expired', function () {
+    $user = User::factory()->create();
+
+    $result = FileRecord::factory()->for($user)->create([
+        'expires_at' => now()->addDay(),
+        'status' => FileStatus::Expired,
+    ]);
+
+    ConversionJob::factory()->for($user)->create([
+        'status' => ConversionStatus::Completed,
+        'result_file_id' => $result->id,
+    ]);
+
+    $this->actingAs($user);
+
+    Livewire::test(RecentConversionsTable::class)
+        ->assertDontSee('Download')
+        ->assertSee('Expired');
+});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/Feature/Livewire/RecentConversionsTableTest.php` around lines 381 -
399, Add a second test to cover the status-driven expiration case: in
RecentConversionsTableTest (next to the existing test it('does not show download
action for completed conversion with expired result')), create a FileRecord via
FileRecord::factory() with 'status' => FileStatus::Expired and an 'expires_at'
in the future, then create the associated ConversionJob with
ConversionStatus::Completed pointing to that result, act as the user and assert
Livewire::test(RecentConversionsTable::class)
->assertDontSee('Download')->assertSee('Expired'); this ensures the isExpired()
contract is exercised for FileStatus::Expired as well as time-based expiry.
tests/Feature/Jobs/CleanupExpiredFilesJobTest.php (1)

83-95: ⚡ Quick win

Also assert status is updated when file is already missing.

The no-throw assertion is good, but this path should still verify record state transitions to FileStatus::Expired.

Suggested assertion addition
 it('does not crash when physical file is already missing', function () {
@@
-    FileRecord::factory()->create([
+    $file = FileRecord::factory()->create([
         'stored_path' => 'uploads/already-gone.png',
         'status' => FileStatus::Analyzed,
         'expires_at' => now()->subMinute(),
     ]);
 
     expect(fn () => app(CleanupExpiredFilesJob::class)->handle())->not->toThrow(Throwable::class);
+    expect($file->fresh()->status)->toBe(FileStatus::Expired);
 });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/Feature/Jobs/CleanupExpiredFilesJobTest.php` around lines 83 - 95, Add
an assertion after calling app(CleanupExpiredFilesJob::class)->handle() to load
the FileRecord created earlier and assert its status has been updated to
FileStatus::Expired; specifically, retrieve the FileRecord (e.g.,
FileRecord::first() or refresh the model) and assert ->status ===
FileStatus::Expired to confirm the job marks missing files as expired while
still not throwing in CleanupExpiredFilesJob::handle.
tests/Feature/Api/V1/ConversionDownloadEndpointTest.php (1)

66-70: ⚡ Quick win

Assert expired_at in the 410 payload contract.

These tests validate error.code, but they don’t verify the expired_at field promised by the API contract for expired results.

Suggested test tightening
     $this->withToken($token)
         ->getJson("/api/v1/conversions/{$job->id}/download")
         ->assertStatus(410)
-        ->assertJsonPath('error.code', 'result_expired');
+        ->assertJsonPath('error.code', 'result_expired')
+        ->assertJsonStructure([
+            'error' => [
+                'code',
+                'details' => ['expired_at'],
+            ],
+        ]);

@@
     $this->withToken($token)
         ->getJson("/api/v1/conversions/{$job->id}/download")
         ->assertStatus(410)
-        ->assertJsonPath('error.code', 'result_expired');
+        ->assertJsonPath('error.code', 'result_expired')
+        ->assertJsonStructure([
+            'error' => [
+                'code',
+                'details' => ['expired_at'],
+            ],
+        ]);

Also applies to: 88-92

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/Feature/Api/V1/ConversionDownloadEndpointTest.php` around lines 66 -
70, The test currently only asserts the error.code for expired results; update
ConversionDownloadEndpointTest to also assert the presence and format/value of
the expired_at field in the 410 payload by adding an assertion like
assertJsonPath('error.expired_at', ...) after the getJson call (repeat the same
change for the other assertion block at lines 88-92). Ensure you reference the
same $job->id request and use an appropriate check (presence/non-empty or
ISO8601 timestamp validation) for error.expired_at so the contract is fully
verified.
tests/Feature/Console/CleanupExpiredFilesCommandTest.php (1)

5-15: ⚡ Quick win

Validate command mode behavior, not only output.

These tests should also lock mode semantics: default run dispatches cleanup job, while --sync executes cleanup inline.

Suggested stronger assertions
+use App\Jobs\CleanupExpiredFilesJob;
+use Illuminate\Support\Facades\Queue;
+
 it('runs expired files cleanup command successfully', function () {
+    Queue::fake();
+
     $this->artisan('files:cleanup-expired')
         ->expectsOutputToContain('Expired files cleanup completed')
         ->assertExitCode(0);
+
+    Queue::assertPushed(CleanupExpiredFilesJob::class);
 });
 
 it('runs cleanup synchronously with --sync flag', function () {
     $this->artisan('files:cleanup-expired --sync')
         ->expectsOutputToContain('Expired files cleanup completed')
         ->assertExitCode(0);
+
+    // Add a small fixture + side-effect assertion here (e.g., expired file/status updated)
+    // to prove inline execution path.
 });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/Feature/Console/CleanupExpiredFilesCommandTest.php` around lines 5 -
15, Update the two tests to assert mode semantics in addition to output: for the
"runs expired files cleanup command successfully" case, call Bus::fake() before
running $this->artisan('files:cleanup-expired') and assert the cleanup job was
dispatched (e.g., Bus::assertDispatched(CleanupExpiredFilesJob::class) or your
job class); for the "runs cleanup synchronously with --sync flag" case, call
Bus::fake() before $this->artisan('files:cleanup-expired --sync') and assert the
job was NOT dispatched (Bus::assertNotDispatched(...)) and instead verify the
inline behavior happened by spying/mocking the cleanup service or asserting the
job handler was executed (e.g., Mockery::spy(CleanupService::class) or asserting
a method called on the service used by the job).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/Console/Commands/CleanupExpiredFilesCommand.php`:
- Around line 21-25: The current info message always says "Expired files cleanup
completed." even when you dispatch asynchronously; change the command to check
the sync option (use $this->option('sync') or the existing option name) and
branch: when sync is true run the job synchronously (e.g., use
dispatchNow/dispatch_sync or call the job's handle) and then call
$this->info('Expired files cleanup completed.'); otherwise dispatch via
CleanupExpiredFilesJob::dispatch() and call $this->info('Expired files cleanup
queued.'); update the message near CleanupExpiredFilesJob::dispatch() and ensure
the sync branch triggers the synchronous dispatch path.

In `@app/Jobs/CleanupExpiredFilesJob.php`:
- Around line 25-29: The job currently sets $file->forceFill(['status' =>
FileStatus::Expired])->save() regardless of whether
Storage::disk('local')->delete($file->stored_path) succeeded, which can leave
orphaned files; update CleanupExpiredFilesJob so that you attempt deletion of
$file->stored_path and only mark the DB record as Expired when deletion returns
true (or no exception), otherwise leave status unchanged (or set to a transient
failure state), and log or record the deletion failure; ensure you handle both a
false return value and exceptions from Storage::disk('local')->delete and
reference the $file->stored_path, Storage::disk('local')->delete(...) call, and
the forceFill(['status' => FileStatus::Expired])->save() statement when making
the change.

In `@app/Services/Files/FileRetentionPolicy.php`:
- Around line 21-24: The current code casts the value from
$this->features->limit($user, 'retention_days') to int before validating,
allowing values like "1.5" to pass; instead, retrieve the raw value (e.g. $raw =
$this->features->limit($user, 'retention_days')), validate it is a strict
positive integer (use ctype_digit($raw) or filter_var($raw, FILTER_VALIDATE_INT)
and check > 0), and if validation fails throw
InvalidRetentionPolicyException::forInvalidDays($raw); only after validation
convert to int (e.g. $days = (int) $raw) and continue; update the code in
FileRetentionPolicy where $days is assigned/validated to follow this flow.

In `@resources/views/livewire/recent-conversions-table.blade.php`:
- Around line 67-70: Precompute expiry label text and CSS class in the
RecentConversionsTable Livewire component instead of calling business logic in
Blade: add a property (e.g. $expirationMeta) and a helper on
RecentConversionsTable (e.g. computeExpirationMeta() or expirationMetaFor($job))
that iterates jobs and for each job uses the existing
resultExpirationLabel($job) and the resultFile?->isExpired() check to store
['label'=>..., 'class'=> 'text-red-500' or 'text-[var(--ca-muted)]'] keyed by
job id; then update the Blade to only render the prepared values (use
$expirationMeta[$job->id]['label'] and $expirationMeta[$job->id]['class']),
removing any isExpired logic from the view.

---

Outside diff comments:
In `@app/Http/Controllers/Api/V1/ConversionController.php`:
- Around line 70-117: Move the download eligibility and expiry decision tree out
of ConversionController::download into a new Action (e.g.,
app/Actions/DetermineDownloadEligibility or DownloadEligibilityAction) that
accepts the authenticated user and a ConversionJob (or conversionId) and returns
a structured result/enum (allowed, not_ready, missing_result,
expired_with_timestamp, missing_file) plus any metadata (expires_at,
stored_path). In the Action implement the existing checks: ConversionJob
existence, ownership verification (you can call OwnershipGuard from the Action
or accept the pre-validated user and conversion), status !==
ConversionStatus::Completed, null resultFile, resultFile->isExpired() and
Storage::disk('local')->exists($resultFile->stored_path), and map each outcome
to a clear payload. Update ConversionController::download to be
orchestration-only: call the Action, then translate the Action's outcome into
the appropriate JSON response codes/messages (422, 404, 410, etc.) and return
the file stream when allowed. Ensure the Action and controller use the same
unique symbols (ConversionJob, ConversionStatus, OwnershipGuard,
resultFile->isExpired, Storage::disk) so locating logic is straightforward.

---

Nitpick comments:
In `@tests/Feature/Api/V1/ConversionDownloadEndpointTest.php`:
- Around line 66-70: The test currently only asserts the error.code for expired
results; update ConversionDownloadEndpointTest to also assert the presence and
format/value of the expired_at field in the 410 payload by adding an assertion
like assertJsonPath('error.expired_at', ...) after the getJson call (repeat the
same change for the other assertion block at lines 88-92). Ensure you reference
the same $job->id request and use an appropriate check (presence/non-empty or
ISO8601 timestamp validation) for error.expired_at so the contract is fully
verified.

In `@tests/Feature/Console/CleanupExpiredFilesCommandTest.php`:
- Around line 5-15: Update the two tests to assert mode semantics in addition to
output: for the "runs expired files cleanup command successfully" case, call
Bus::fake() before running $this->artisan('files:cleanup-expired') and assert
the cleanup job was dispatched (e.g.,
Bus::assertDispatched(CleanupExpiredFilesJob::class) or your job class); for the
"runs cleanup synchronously with --sync flag" case, call Bus::fake() before
$this->artisan('files:cleanup-expired --sync') and assert the job was NOT
dispatched (Bus::assertNotDispatched(...)) and instead verify the inline
behavior happened by spying/mocking the cleanup service or asserting the job
handler was executed (e.g., Mockery::spy(CleanupService::class) or asserting a
method called on the service used by the job).

In `@tests/Feature/Jobs/CleanupExpiredFilesJobTest.php`:
- Around line 83-95: Add an assertion after calling
app(CleanupExpiredFilesJob::class)->handle() to load the FileRecord created
earlier and assert its status has been updated to FileStatus::Expired;
specifically, retrieve the FileRecord (e.g., FileRecord::first() or refresh the
model) and assert ->status === FileStatus::Expired to confirm the job marks
missing files as expired while still not throwing in
CleanupExpiredFilesJob::handle.

In `@tests/Feature/Livewire/RecentConversionsTableTest.php`:
- Around line 381-399: Add a second test to cover the status-driven expiration
case: in RecentConversionsTableTest (next to the existing test it('does not show
download action for completed conversion with expired result')), create a
FileRecord via FileRecord::factory() with 'status' => FileStatus::Expired and an
'expires_at' in the future, then create the associated ConversionJob with
ConversionStatus::Completed pointing to that result, act as the user and assert
Livewire::test(RecentConversionsTable::class)
->assertDontSee('Download')->assertSee('Expired'); this ensures the isExpired()
contract is exercised for FileStatus::Expired as well as time-based expiry.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 68a43249-c909-4429-b4c8-26b6e0026214

📥 Commits

Reviewing files that changed from the base of the PR and between fb78af6 and 6297662.

📒 Files selected for processing (22)
  • app/Console/Commands/CleanupExpiredFilesCommand.php
  • app/Exceptions/Files/InvalidRetentionPolicyException.php
  • app/Http/Controllers/Api/V1/ConversionController.php
  • app/Jobs/CleanupExpiredFilesJob.php
  • app/Livewire/RecentConversionsTable.php
  • app/Models/FileRecord.php
  • app/Services/Files/FileRetentionPolicy.php
  • resources/views/errors/410.blade.php
  • resources/views/livewire/recent-conversions-table.blade.php
  • routes/console.php
  • tests/Feature/Api/V1/ConversionDownloadEndpointTest.php
  • tests/Feature/Console/CleanupExpiredFilesCommandTest.php
  • tests/Feature/ConversionDownloadTest.php
  • tests/Feature/Files/FileRecordTest.php
  • tests/Feature/Files/StoreUploadedFileActionTest.php
  • tests/Feature/Jobs/CleanupExpiredFilesJobTest.php
  • tests/Feature/Jobs/ProcessConversionJobTest.php
  • tests/Feature/Livewire/RecentConversionsTableTest.php
  • tests/Feature/Retention/FileRetentionLifecycleTest.php
  • tests/Pest.php
  • tests/Unit/Jobs/CleanupExpiredFilesJobSkeletonTest.php
  • tests/Unit/Services/FileRetentionPolicyTest.php

Comment thread app/Console/Commands/CleanupExpiredFilesCommand.php Outdated
Comment thread app/Jobs/CleanupExpiredFilesJob.php
Comment thread app/Services/Files/FileRetentionPolicy.php Outdated
Comment thread resources/views/livewire/recent-conversions-table.blade.php Outdated
… validation, Blade logic

- CleanupExpiredFilesCommand: print 'queued' for async, 'completed' for --sync
- CleanupExpiredFilesJob: only mark record Expired after confirmed physical
  deletion; missing files (exists=false) still marked expired; failed deletes
  are logged and skipped so the file gets retried on the next run
- FileRetentionPolicy: validate retention_days with filter_var before casting
  to prevent values like '1.5' silently truncating to 1
- InvalidRetentionPolicyException: accept int|string|null raw value in message
- RecentConversionsTable: replace resultExpirationLabel + inline isExpired()
  Blade logic with expirationMetaFor() returning {label, class}; no domain
  calls left in the template
- Tests: Bus::fake assertions in command test; expired_at assertion in API
  download test; status assertion in missing-file cleanup test; status-driven
  expiration test in Livewire table test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@menvil menvil merged commit 0d17ebe into main Jun 5, 2026
5 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release Triggers AI code review (CodeRabbit, Cubic)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant