Release v0.1.9 — Phase 9: Conversion Job Core (CONV-112–CONV-129)#123
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…126/CONV-128) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…CONV-127/CONV-129) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 36 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces a complete file conversion pipeline enabling users to convert files between formats asynchronously. The system creates and tracks conversion jobs, dispatches them to a queue, executes conversions via pluggable drivers, and persists results as new file records with checksums and metadata. ChangesConversion Pipeline Implementation
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateAction as CreateConversionJobAction
participant ConverterReg as ConverterRegistry
participant Queue as Queue Dispatcher
participant Job as ProcessConversionJob
participant DriverReg as ConverterDriverRegistry
participant Driver as ConverterDriver
participant Recorder as RecordConversionResultFileAction
participant DB as Database
User->>CreateAction: handle(user, file, target_format, options)
CreateAction->>ConverterReg: normalize & find converter
ConverterReg-->>CreateAction: converter instance
CreateAction->>ConverterReg: validate(options)
ConverterReg-->>CreateAction: validation ok
CreateAction->>DB: create ConversionJob (Queued)
DB-->>CreateAction: job record
CreateAction->>Queue: push ProcessConversionJob
Queue-->>CreateAction: queued
CreateAction-->>User: ConversionJob instance
Queue->>Job: handle(conversionJobId)
Job->>DB: load ConversionJob
DB-->>Job: job record
Job->>DB: update status to Processing
Job->>DriverReg: findOrFail(converter_key)
DriverReg-->>Job: ConverterDriver instance
Job->>Driver: convert(ConversionContext)
Driver-->>Job: ConversionResult
Job->>Recorder: handle(user, result)
Recorder->>Recorder: compute SHA-256
Recorder->>DB: create FileRecord (Analyzed)
DB-->>Recorder: FileRecord instance
Recorder-->>Job: FileRecord
Job->>DB: update job (Completed, result_file_id, progress=100)
alt Conversion Fails
Job->>DB: update job (Failed, error_code, error_message)
end
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
database/factories/ConversionJobFactory.php (1)
23-24: ⚡ Quick winKeep factory ownership consistent between
user_idandsource_file_id.Line 23 and Line 24 currently build unrelated records, so tests can generate a
ConversionJobwhose source file belongs to a different user. Tiesource_file_idto the same user to keep fixtures domain-valid.Proposed diff
public function definition(): array { return [ 'user_id' => User::factory(), - 'source_file_id' => FileRecord::factory(), + 'source_file_id' => FileRecord::factory()->state( + fn (array $attributes): array => ['user_id' => $attributes['user_id']] + ), 'result_file_id' => null, 'source_format' => 'png', 'target_format' => 'jpg', 'converter_key' => 'png_to_jpg',🤖 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 `@database/factories/ConversionJobFactory.php` around lines 23 - 24, ConversionJobFactory currently creates an independent User::factory() for 'user_id' and an unrelated FileRecord::factory() for 'source_file_id', which can produce a source file owned by a different user; fix this by creating a single shared user factory instance and use it for both fields (e.g. assign $user = User::factory() to 'user_id' and make 'source_file_id' use FileRecord::factory()->state(['user_id' => $user]) or, if FileRecord has a user relation, FileRecord::factory()->for($user) ), updating the ConversionJobFactory so 'user_id' and FileRecord::factory reference the same $user.
🤖 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/Actions/Conversions/CreateConversionJobAction.php`:
- Around line 56-62: CreateConversionJobAction is persisting the converter key
from Converter::key() (e.g. "png:jpg") into ConversionJob.converter_key, but
ProcessConversionJob looks up drivers via ConverterDriverRegistry::findOrFail
which expects driver-style keys (e.g. "png_to_jpg"); change
CreateConversionJobAction to store the registry-compatible key instead of the
raw converter key—e.g. transform $converter->key() into the driver key format
expected by ConverterDriverRegistry (replace ":" with "_" or call a
normalization helper on the registry) before saving (affecting the
'converter_key' assignment), so
ProcessConversionJob::findOrFail($job->converter_key) will resolve correctly.
In `@app/Actions/Conversions/RecordConversionResultFileAction.php`:
- Around line 22-31: The code in RecordConversionResultFileAction currently
casts Storage::disk('local')->get($result->path) to string which masks missing
files and lets you store a checksum of an empty string; remove the (string)
cast, check the raw return value from Storage::disk('local')->get($result->path)
and throw a clear exception (e.g., RuntimeException or a domain-specific
exception) if it is null before computing hash('sha256', ...), then proceed to
call FileRecord::create(...) using the validated contents and existing $result
fields.
In `@app/Jobs/ProcessConversionJob.php`:
- Around line 67-76: The catch-all in ProcessConversionJob (catch (Throwable
$exception)) swallows all failures after updating the conversion via
forceFill(...) and report($exception), preventing Laravel from retrying
transient infra errors; change the error handling in ProcessConversionJob so you
only catch domain/terminal exceptions (e.g. specific ConversionDomainException
types) and handle them by updating the conversion record, but for other
exceptions rethrow the exception or call $this->fail($exception) / release() so
the queue can retry; locate the try/catch in the ProcessConversionJob class and
replace the broad Throwable catch with targeted exception types or add logic to
detect infrastructure errors and rethrow or call $this->fail($exception) after
reporting.
---
Nitpick comments:
In `@database/factories/ConversionJobFactory.php`:
- Around line 23-24: ConversionJobFactory currently creates an independent
User::factory() for 'user_id' and an unrelated FileRecord::factory() for
'source_file_id', which can produce a source file owned by a different user; fix
this by creating a single shared user factory instance and use it for both
fields (e.g. assign $user = User::factory() to 'user_id' and make
'source_file_id' use FileRecord::factory()->state(['user_id' => $user]) or, if
FileRecord has a user relation, FileRecord::factory()->for($user) ), updating
the ConversionJobFactory so 'user_id' and FileRecord::factory reference the same
$user.
🪄 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: eeab42dc-6594-480a-86c4-00d6763f4e2e
📒 Files selected for processing (29)
app/Actions/Conversions/CreateConversionJobAction.phpapp/Actions/Conversions/RecordConversionResultFileAction.phpapp/Enums/ConversionStatus.phpapp/Jobs/ProcessConversionJob.phpapp/Models/ConversionJob.phpapp/Models/FileRecord.phpapp/Models/User.phpapp/Providers/ConverterServiceProvider.phpapp/Support/Conversions/Contracts/ConverterDriver.phpapp/Support/Conversions/ConverterDriverRegistry.phpapp/Support/Conversions/DTO/ConversionContext.phpapp/Support/Conversions/DTO/ConversionResult.phpapp/Support/Conversions/Exceptions/MissingConverterDriverException.phpapp/Support/Conversions/Exceptions/UnsupportedConversionException.phpdatabase/factories/ConversionJobFactory.phpdatabase/migrations/2026_05_31_000001_create_conversion_jobs_table.phptests/Fakes/Conversions/FakeConverterDriver.phptests/Feature/Actions/CreateConversionJobActionTest.phptests/Feature/Actions/RecordConversionResultFileActionTest.phptests/Feature/Jobs/ProcessConversionJobTest.phptests/Feature/Models/ConversionJobTest.phptests/Unit/Actions/CreateConversionJobActionSkeletonTest.phptests/Unit/Conversions/ConversionContextTest.phptests/Unit/Conversions/ConversionResultTest.phptests/Unit/Conversions/ConverterDriverRegistryTest.phptests/Unit/Conversions/ConverterDriverTest.phptests/Unit/Conversions/FakeConverterDriverTest.phptests/Unit/Enums/ConversionStatusTest.phptests/Unit/Jobs/ProcessConversionJobSkeletonTest.php
Comment on lines
+67
to
+76
| } catch (Throwable $exception) { | ||
| $job->forceFill([ | ||
| 'status' => ConversionStatus::Failed, | ||
| 'error_code' => Str::snake(class_basename($exception)), | ||
| 'error_message' => $exception->getMessage(), | ||
| 'completed_at' => now(), | ||
| ])->save(); | ||
|
|
||
| report($exception); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for tries/backoff/timeout/failed() conventions on existing jobs.
fd -e php . app/Jobs 2>/dev/null | xargs rg -nP '\$tries|\$backoff|\bfunction backoff\b|\$timeout|\bfunction failed\b' -C2Repository: menvil/FileConverter
Length of output: 48
Avoid swallowing Throwable in ProcessConversionJob to allow queue retries
- In
app/Jobs/ProcessConversionJob.php(lines 67-76),catch (Throwable $exception)updates the conversion record, callsreport($exception), and then exits without rethrowing or calling$this->fail(), so Laravel will treat the job as handled and won’t retry transient failures. - Narrow the catch to terminal domain errors and rethrow (or otherwise fail/release) infrastructure-related exceptions.
🤖 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/Jobs/ProcessConversionJob.php` around lines 67 - 76, The catch-all in
ProcessConversionJob (catch (Throwable $exception)) swallows all failures after
updating the conversion via forceFill(...) and report($exception), preventing
Laravel from retrying transient infra errors; change the error handling in
ProcessConversionJob so you only catch domain/terminal exceptions (e.g. specific
ConversionDomainException types) and handle them by updating the conversion
record, but for other exceptions rethrow the exception or call
$this->fail($exception) / release() so the queue can retry; locate the try/catch
in the ProcessConversionJob class and replace the broad Throwable catch with
targeted exception types or add logic to detect infrastructure errors and
rethrow or call $this->fail($exception) after reporting.
… missing result file, share factory user - CreateConversionJobAction stores the driver-style converter_key (e.g. png_to_jpg) so ProcessConversionJob's ConverterDriverRegistry lookup resolves. - RecordConversionResultFileAction throws when the result file is missing instead of hashing an empty string. - ConversionJobFactory keeps the source file owned by the same user as the job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 1, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release v0.1.9 — Phase 9: Conversion Job Core (CONV-112 → CONV-129)
Release branch
release/v0.1.9-phase09-conversion-job-core→main.mainis current withdevelop, so this PR contains exactly the 16 Phase 9 commits (CONV-113 → CONV-127, plus a Pint formatting fixup). Same commits as PR #122 (feature →develop), targetingmainfor the release.Phase 9 (backend conversion-job core)
ConversionStatusenum,conversion_jobstable,ConversionJobmodel + factory + relationshipsConversionContext/ConversionResultDTOsConverterDriverinterface,ConverterDriverRegistry, test-onlyFakeConverterDriverRecordConversionResultFileAction,CreateConversionJobActionProcessConversionJobqueue job:queued → processing → completedand safe→ failedpathsjobs/failed_jobsmigration +QUEUE_CONNECTION=databaseVerification
composer test— green (256 tests / 700 assertions)composer lint(Pint) — cleannpm run build— passesNotes
develop+ Phase 9). PR Phase 9: Conversion Job Core (CONV-112–CONV-129) #122 (feature →develop) is still open; merge order is at your discretion.v0.1.9-phase09-conversion-job-core.