release/v0.1.11: Phase 11 — Convert UI Flow#162
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on-test CONV-147: Add convert button test
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…button-to-create-conversion-job-action CONV-148: Connect convert button to CreateConversionJobAction
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nvert-protection CONV-149: Add duplicate convert protection
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tate-test CONV-150: Add converting state test
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ting-state-ui CONV-151: Implement converting state UI
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…olling-test CONV-152: Add job status polling test
refreshConversionStatus and wire:poll.2s implemented in CONV-151. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…atus-polling CONV-153: Implement job status polling
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ate-test CONV-154: Add completed state test
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ted-state-ui CONV-155: Implement completed state UI
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-test CONV-156: Add failed state test
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-state-ui CONV-157: Implement failed state UI
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oad-route-test CONV-158: Add result download route test
Controller implemented in CONV-155. Fixed unused import lint. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-download-route CONV-159: Implement result download route
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…her-action CONV-160: Add convert another action
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-different-settings-action CONV-161: Add convert with different settings action
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ed-ui-handling CONV-162: Add result expired UI handling
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-integration-test CONV-163: Add convert flow integration test
📝 WalkthroughWalkthroughThis PR implements a complete conversion result download feature, integrating job tracking into the Livewire dashboard, adding a gated download controller, and extending both the model layer and Blade template to support the full conversion lifecycle with authorization, expiration, and state management. ChangesConversion Result Download and Lifecycle Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
2 issues found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/Http/Controllers/DownloadConversionResultController.php (1)
22-24: ⚡ Quick winReuse
FileRecord::isExpired()instead of duplicating the expiry check.This PR introduces
FileRecord::isExpired(), which encodes the exact same logic. Using it here keeps the expiry rule in one place and avoids divergence later.♻️ Proposed refactor
- if ($file->expires_at !== null && $file->expires_at->isPast()) { - abort(410); - } + if ($file->isExpired()) { + abort(410); + }🤖 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/DownloadConversionResultController.php` around lines 22 - 24, Replace the duplicated expiry check in DownloadConversionResultController with the centralized method: instead of directly checking $file->expires_at and isPast(), call FileRecord::isExpired() (or $file->isExpired() if it's an instance method) to decide whether to abort(410); update the conditional to use that method so the expiry logic is maintained in FileRecord::isExpired().tests/Feature/ConversionDownloadTest.php (1)
10-101: 💤 Low valueConsider covering the missing-result-file and missing-on-disk 404 branches.
The controller has two additional 404 guards not exercised here: a completed job with
result_file_idnull (Line 18), and a result file whosestored_pathis absent on thelocaldisk (Line 26). Adding cases for these would lock in the full gating behavior.🤖 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/ConversionDownloadTest.php` around lines 10 - 101, Add two tests to cover the controller's 404 branches: (1) create a completed ConversionJob (ConversionJob::factory()->for($user)->completed()->create()) with result_file_id set to null and assert the GET route('conversions.download', $job) returns 404; (2) create a FileRecord in the DB with 'stored_path' set (FileRecord::factory()->for($user)->create([...])) but do NOT put the file into Storage::disk('local') (use Storage::fake('local') without writing the file) and attach its id to a completed ConversionJob so a download request to route('conversions.download', $job) asserts 404; place these alongside the existing tests to exercise the missing-result-file and missing-on-disk branches.resources/views/livewire/dashboard/dashboard-converter.blade.php (1)
241-258: Polling has no terminal timeout for stuck jobs.
wire:poll.2s="refreshConversionStatus"only stops once the job reachescompleted/failed. A job stuck inqueued/processing(e.g., worker outage) will poll forever, keeping the user on this screen with no recovery path. Consider a max-wait threshold (e.g., transition to a "taking longer than expected" / failed state after N seconds) to bound the loop and improve resilience.🤖 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 `@resources/views/livewire/dashboard/dashboard-converter.blade.php` around lines 241 - 258, The converting view currently uses wire:poll.2s="refreshConversionStatus" with no terminal timeout, so stuck jobs poll forever; update the refreshConversionStatus Livewire action (and/or the component state used by $step) to implement a max-wait threshold (e.g., track a started_at timestamp or increment a retry counter on refresh) and transition $step from 'converting' to a timeout state like 'taking_too_long' or 'failed' after N seconds or N polls; ensure the Blade template ($step === 'converting') and any UI for currentJob display handle the new state so the poll stops (remove or stop polling when threshold reached) and the user sees recovery options.
🤖 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/Http/Controllers/DownloadConversionResultController.php`:
- Around line 14-18: The strict identity check in
DownloadConversionResultController::__invoke between $conversion->user_id and
$request->user()->id can fail due to type differences; fix it by normalizing
types: either add a cast for user_id in the ConversionJob model (add protected
$casts['user_id'] = 'integer' to app/Models/ConversionJob.php) or change the
authorization check in DownloadConversionResultController::__invoke to compare
ints (e.g., (int) $conversion->user_id === (int) $request->user()->id) so the
user_id comparison is type-safe.
In `@app/Livewire/Dashboard/DashboardConverter.php`:
- Around line 219-224: Wrap the call to CreateConversionJobAction::handle()
inside DashboardConverter::convert() in a try/catch similar to storeUpload():
catch UnsupportedConversionException and surface its message to the user (e.g.,
set a Livewire error/message and reset the component step/state to a safe
fallback), then catch any other Throwable to log the exception and show a
generic user-facing error and reset state; explicitly reference
CreateConversionJobAction::handle(), UnsupportedConversionException,
ConverterRegistry::find(), validateOptions(), ConversionJob::create(), and
ProcessConversionJob::dispatch() so you guard against failures from those calls
and ensure convert() fails gracefully rather than bubbling uncaught exceptions.
---
Nitpick comments:
In `@app/Http/Controllers/DownloadConversionResultController.php`:
- Around line 22-24: Replace the duplicated expiry check in
DownloadConversionResultController with the centralized method: instead of
directly checking $file->expires_at and isPast(), call FileRecord::isExpired()
(or $file->isExpired() if it's an instance method) to decide whether to
abort(410); update the conditional to use that method so the expiry logic is
maintained in FileRecord::isExpired().
In `@resources/views/livewire/dashboard/dashboard-converter.blade.php`:
- Around line 241-258: The converting view currently uses
wire:poll.2s="refreshConversionStatus" with no terminal timeout, so stuck jobs
poll forever; update the refreshConversionStatus Livewire action (and/or the
component state used by $step) to implement a max-wait threshold (e.g., track a
started_at timestamp or increment a retry counter on refresh) and transition
$step from 'converting' to a timeout state like 'taking_too_long' or 'failed'
after N seconds or N polls; ensure the Blade template ($step === 'converting')
and any UI for currentJob display handle the new state so the poll stops (remove
or stop polling when threshold reached) and the user sees recovery options.
In `@tests/Feature/ConversionDownloadTest.php`:
- Around line 10-101: Add two tests to cover the controller's 404 branches: (1)
create a completed ConversionJob
(ConversionJob::factory()->for($user)->completed()->create()) with
result_file_id set to null and assert the GET route('conversions.download',
$job) returns 404; (2) create a FileRecord in the DB with 'stored_path' set
(FileRecord::factory()->for($user)->create([...])) but do NOT put the file into
Storage::disk('local') (use Storage::fake('local') without writing the file) and
attach its id to a completed ConversionJob so a download request to
route('conversions.download', $job) asserts 404; place these alongside the
existing tests to exercise the missing-result-file and missing-on-disk branches.
🪄 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: 151de432-ca0e-44cc-9486-1d38a3c791de
📒 Files selected for processing (10)
app/Http/Controllers/DownloadConversionResultController.phpapp/Livewire/Dashboard/DashboardConverter.phpapp/Models/FileRecord.phpdatabase/factories/ConversionJobFactory.phpdatabase/factories/FileRecordFactory.phpresources/views/livewire/dashboard/dashboard-converter.blade.phproutes/web.phptests/Feature/ConversionDownloadTest.phptests/Feature/ConversionFlowTest.phptests/Feature/Livewire/DashboardConverterConvertTest.php
…ety, polling timeout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Phase 11 review fixes: convert error handling, stale job ID, type safety, polling timeout
Phase 11 — Convert UI Flow (CONV-147–CONV-163)
Connects the dashboard converter UI to the backend job pipeline from Phase 9/10.
What's new
Convert flow:
convert()method inDashboardConverter— creates ConversionJob viaCreateConversionJobAction, guards duplicate clicksconvert(pre-flight) →converting(spinner + polling) →completed/failedUI states:
converting: spinner, source→target labels,wire:poll.2sonrefreshConversionStatuscompleted: Done message, filename, Download link, Change settings, Convert anothercompleted (expired): "This result has expired" message, no Download buttonfailed: user-friendly message, raw error hidden, Change settings / Try another fileDownload route:
GET /conversions/{conversion}/download— owner-only, completed-only, expiry check, 403/404/410Actions:
convertAnother()— full state reset to upload stepconvertWithDifferentSettings()— keeps file/target/options, clears job, returns to settingsFactory states added:
ConversionJobFactory:queued(),processing(),completed(),failed()FileRecordFactory:expired()Model:
FileRecord::isExpired()helperTest plan
composer test— 286/286 testscomposer lint— passesnpm run build— passes🤖 Generated with Claude Code
Summary by cubic
Connects the dashboard Convert UI to the backend pipeline so users can convert a file and download the result. Addresses CONV-147–CONV-163 with clear states, polling with timeout, better error handling, and a secure download route.
convert(), creates aConversionJob, guards duplicate clicks, and shows friendly errors (unsupported conversion or generic failure).wire:poll.2sonrefreshConversionStatus) updates status tocompleted/failedand times out after 60 polls with a helpful message.convertAnother()andconvertWithDifferentSettings()(clears stale job ID and returns to settings when possible).GET /conversions/{conversion}/download(owner-only, completed-only, result-present, expiry-aware; returns 403/404/410; preserves content type).ConversionJobFactory(queued(),processing(),completed(),failed()),FileRecordFactory::expired(), andFileRecord::isExpired().Written for commit 32e23ba. Summary will update on new commits.
Summary by CodeRabbit