Skip to content

Phase 11 review fixes: convert error handling, stale job ID, type safety, polling timeout#163

Merged
menvil merged 1 commit into
developfrom
fix/phase11-review-fixes
Jun 2, 2026
Merged

Phase 11 review fixes: convert error handling, stale job ID, type safety, polling timeout#163
menvil merged 1 commit into
developfrom
fix/phase11-review-fixes

Conversation

@menvil

@menvil menvil commented Jun 2, 2026

Copy link
Copy Markdown
Owner

Fixes

P1 — Stale currentConversionJobId in convertWithDifferentSettings()

  • currentConversionJobId = null moved before the early-return guard so it's always cleared regardless of file/format state
  • Prevents the stale ID from blocking future convert() calls

P2 — convert() unhandled exceptions → 500

  • UnsupportedConversionException → user-friendly message + step reset to 'format'
  • Generic Throwable → user-friendly message + stay on 'convert' step
  • convertError property surfaces the message to the Blade template

Inline — type-unsafe === in download controller

  • (int) $conversion->user_id === (int) $request->user()->id — safe under SQLite string coercion
  • Expiry check replaced with $file->isExpired() (centralized in FileRecord)

Nitpick — infinite polling for stuck jobs

  • $pollCount property added; refreshConversionStatus() increments it each call
  • After 60 polls (~2 min at 2s interval) → step = 'failed' with timeout message
  • pollCount reset to 0 on each new convert() call and convertWithDifferentSettings()

Nitpick — missing download controller test branches

  • Test: completed job with result_file_id = null → 404
  • Test: FileRecord exists in DB but file missing from storage → 404

Test plan

  • composer test passes — 290/290
  • composer lint passes

🤖 Generated with Claude Code


Summary by cubic

Improves conversion flow reliability by clearing stale job IDs, adding user-friendly error handling, and adding a polling timeout. Also makes download checks type-safe and centralizes expiry logic.

  • Bug Fixes
    • Clear currentConversionJobId before guards in convertWithDifferentSettings() to avoid blocking future convert() calls.
    • Handle UnsupportedConversionException and generic errors in convert() with convertError; route back to 'format' for unsupported conversions, otherwise stay on 'convert'.
    • Add pollCount with a 60-attempt cap (~2 min) to stop infinite polling; reset on new conversions and when changing settings.
    • In the download controller, cast IDs to int for safe comparison and use $file->isExpired(); return 410 for expired files and 404 when missing.

Written for commit 63c7984. Summary will update on new commits.

Review in cubic

…ety, polling timeout

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

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • release

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f3131ec7-0aaf-4777-ad57-acfd2b323beb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/phase11-review-fixes

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.

@menvil menvil merged commit 32e23ba into develop Jun 2, 2026
4 checks passed
@menvil menvil deleted the fix/phase11-review-fixes branch June 2, 2026 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant