Phase 22 — History Page (CONV-347–CONV-363)#294
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces a conversion history page where authenticated users can view their conversion jobs with filtering, download completed results, and request repeat conversions for jobs with unexpired source files. The feature includes a Livewire component with multi-field filtering, eligibility checks, and comprehensive test coverage. ChangesConversion History Page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 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="resources/views/livewire/conversion-history-table.blade.php">
<violation number="1" location="resources/views/livewire/conversion-history-table.blade.php:101">
P2: Credits display incorrectly hides valid `0` captured amounts by using a truthy check.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![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 (3)
tests/Feature/Livewire/ConversionHistoryTableActionsTest.php (1)
11-26: ⚡ Quick winConsider testing download URL functionality, not just text presence.
These tests verify the "Download" text appears/disappears, but don't confirm the download link points to the correct file or uses the proper route. Text-based assertions won't catch broken URLs or incorrect route parameters.
💡 Example enhancement to verify download link
For the completed non-expired result test, you could add:
->assertSeeHtml('href="' . route('files.download', $result) . '"')This ensures the link targets the correct file download route.
Also applies to: 45-53
🤖 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/ConversionHistoryTableActionsTest.php` around lines 11 - 26, Update the tests that currently only assert the presence of the "Download" text to also assert the actual download link URL is rendered: in the test using Livewire::actingAs(...)->test(ConversionHistoryTable::class) (and the similar test for the expired case), after asserting see/not see, add an assertion that checks for the expected href pointing to route('files.download', $result) so the rendered anchor targets the correct file download route; locate the test using FileRecord::factory() and ConversionJob::factory()->completed() to get the $result and assert the HTML contains the download route URL.tests/Feature/Livewire/ConversionHistoryTableFiltersTest.php (1)
11-169: ⚡ Quick winConsider adding a test for pagination reset on filter change.
The PR objectives state that "Pagination...resets on filter changes," but there's no test verifying this behavior. Consider adding a test that:
- Navigates to page 2
- Changes a filter
- Asserts the component is back on page 1
💡 Example test structure
it('resets pagination when filter changes', function () { $user = User::factory()->create(); // Create enough jobs to span multiple pages (>15) FileRecord::factory() ->count(20) ->for($user) ->create() ->each(fn($file) => ConversionJob::factory() ->for($user) ->for($file, 'sourceFile') ->create() ); Livewire::actingAs($user) ->test(ConversionHistoryTable::class) ->call('gotoPage', 2) ->set('search', 'test') ->assertSet('paginators.page', 1); });🤖 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/ConversionHistoryTableFiltersTest.php` around lines 11 - 169, Add a test that verifies pagination resets on filter change: in a new it('resets pagination when filter changes', ...) create more than the per-page limit (e.g. 20) ConversionJob records for the same user (use FileRecord::factory()->count(20) and ConversionJob::factory() for each), then Livewire::actingAs($user)->test(ConversionHistoryTable::class)->call('gotoPage', 2)->set('search', 'something') and assert the paginator was reset with ->assertSet('paginators.page', 1); reference ConversionHistoryTable, gotoPage, and paginators.page in the test.tests/Feature/Livewire/ConversionHistoryTableTest.php (1)
21-53: ⚡ Quick winAdd eager loading verification to prevent N+1 queries.
The PR objectives explicitly mention "eager-loaded relations (sourceFile, resultFile, creditCharge)," but there's no test verifying eager loading is actually used. Without this, the component could trigger N+1 queries when rendering the table.
💡 Example eager loading test
use Illuminate\Support\Facades\DB; it('eager loads relations to avoid N+1 queries', function () { $user = User::factory()->create(); // Create multiple jobs with related records ConversionJob::factory() ->count(5) ->for($user) ->completed() ->create() ->each(function ($job) use ($user) { ConversionCreditCharge::factory() ->for($user) ->for($job, 'conversionJob') ->create(['status' => 'captured']); }); DB::enableQueryLog(); Livewire::actingAs($user) ->test(ConversionHistoryTable::class) ->assertOk(); $queries = DB::getQueryLog(); // Should have a small, fixed number of queries regardless of job count // Adjust threshold based on expected query count expect(count($queries))->toBeLessThan(10); });🤖 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/ConversionHistoryTableTest.php` around lines 21 - 53, The test lacks verification that ConversionHistoryTable eagerly loads relations (sourceFile, resultFile, creditCharge) to prevent N+1 queries; add a new test in ConversionHistoryTableTest that creates multiple ConversionJob records (via ConversionJob::factory()->count(n)->for($user)...) with associated ConversionCreditCharge and files, enable query logging (DB::enableQueryLog()), render the component with Livewire::actingAs($user)->test(ConversionHistoryTable::class) and then assert the total number of DB queries is small/under a threshold (e.g. <10) to prove relations are eager-loaded; ensure the test references the relations sourceFile, resultFile, and creditCharge so it fails if lazy-loading occurs.
🤖 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/Livewire/ConversionHistoryTable.php`:
- Around line 129-131: The query that fetches $job
(ConversionJob::query()->where('user_id', auth()->id())->find($jobId)) should
eager-load the sourceFile relation to avoid a lazy load when
canConvertAgain($job) accesses $job->sourceFile; update the query to include the
relation (e.g., use with('sourceFile') on the ConversionJob query) so $job is
returned with sourceFile already loaded before calling canConvertAgain.
In `@tests/Feature/Livewire/ConversionHistoryTableActionsTest.php`:
- Around line 55-67: The test currently only asserts visibility; update it to
call the Livewire action ConversionHistoryTable::convertAgain with the created
ConversionJob's id, then assert the browser event 'conversion-repeat-requested'
was dispatched with payload ['conversionJobId' => $job->id] and that the
component redirected to the dashboard route; specifically, capture the created
ConversionJob in a $job variable, invoke ->call('convertAgain', $job->id) on the
Livewire test instance, then add
->assertDispatchedBrowserEvent('conversion-repeat-requested', ['conversionJobId'
=> $job->id]) and ->assertRedirect(route('dashboard')) (or equivalent
assertRedirect to the dashboard) to the test.
---
Nitpick comments:
In `@tests/Feature/Livewire/ConversionHistoryTableActionsTest.php`:
- Around line 11-26: Update the tests that currently only assert the presence of
the "Download" text to also assert the actual download link URL is rendered: in
the test using Livewire::actingAs(...)->test(ConversionHistoryTable::class) (and
the similar test for the expired case), after asserting see/not see, add an
assertion that checks for the expected href pointing to route('files.download',
$result) so the rendered anchor targets the correct file download route; locate
the test using FileRecord::factory() and ConversionJob::factory()->completed()
to get the $result and assert the HTML contains the download route URL.
In `@tests/Feature/Livewire/ConversionHistoryTableFiltersTest.php`:
- Around line 11-169: Add a test that verifies pagination resets on filter
change: in a new it('resets pagination when filter changes', ...) create more
than the per-page limit (e.g. 20) ConversionJob records for the same user (use
FileRecord::factory()->count(20) and ConversionJob::factory() for each), then
Livewire::actingAs($user)->test(ConversionHistoryTable::class)->call('gotoPage',
2)->set('search', 'something') and assert the paginator was reset with
->assertSet('paginators.page', 1); reference ConversionHistoryTable, gotoPage,
and paginators.page in the test.
In `@tests/Feature/Livewire/ConversionHistoryTableTest.php`:
- Around line 21-53: The test lacks verification that ConversionHistoryTable
eagerly loads relations (sourceFile, resultFile, creditCharge) to prevent N+1
queries; add a new test in ConversionHistoryTableTest that creates multiple
ConversionJob records (via ConversionJob::factory()->count(n)->for($user)...)
with associated ConversionCreditCharge and files, enable query logging
(DB::enableQueryLog()), render the component with
Livewire::actingAs($user)->test(ConversionHistoryTable::class) and then assert
the total number of DB queries is small/under a threshold (e.g. <10) to prove
relations are eager-loaded; ensure the test references the relations sourceFile,
resultFile, and creditCharge so it fails if lazy-loading occurs.
🪄 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: 6987d0d0-e3d7-4357-b253-4a391d39f12c
📒 Files selected for processing (9)
app/Livewire/ConversionHistoryTable.phpresources/views/history/index.blade.phpresources/views/livewire/conversion-history-table.blade.phproutes/web.phptests/Feature/History/HistoryPageAccessTest.phptests/Feature/History/HistoryPageSmokeTest.phptests/Feature/Livewire/ConversionHistoryTableActionsTest.phptests/Feature/Livewire/ConversionHistoryTableFiltersTest.phptests/Feature/Livewire/ConversionHistoryTableTest.php
…, strengthen action tests
- Fix P2: credits column used truthy check hiding valid `captured_amount = 0`; changed to explicit !== null check
- Add with('sourceFile') eager-load in convertAgain query to avoid lazy-load when canConvertAgain accesses $job->sourceFile
- Strengthen convertAgain test: call action, assert event dispatched with payload, assert redirect to dashboard
- Strengthen download test: assert rendered href matches route('conversions.download', $job)
- Add pagination reset test: asserts page resets to 1 when search filter changes
- Add N+1 prevention test: asserts query count < 10 for 5 jobs with all relations loaded
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.
Summary
/historyroute (auth-protected) with full conversion history pageConversionHistoryTableLivewire component with user-scoped query, all filter types, row actions, and credit cost columnWhat's included
/historypage — guest redirects to login, auth user gets 200ConversionHistoryTableLivewire component with eager-loadedsourceFile,resultFile,creditCharge<x-badge>(success/warning/danger/purple/neutral)dateFrom/dateTooncreated_atconversion-repeat-requestedevent and redirects to dashboardcaptured_amountonly, never recalculatesTest plan
composer test— 670 tests passingcomposer lint— cleannpm run build— passes/history(redirects to login)/history🤖 Generated with Claude Code
Summary by cubic
Adds an authenticated History page at /history with a filterable conversion table and reuse/download actions for past jobs. Fulfills Linear CONV-347–CONV-363.
New Features
/historyroute (auth-only) withConversionHistoryTablescoped to the current user and eager-loaded relations (sourceFile,resultFile,creditCharge).conversion-repeat-requested); status badges via<x-badge>; credits column shows captured amount; columns include name, formats, size, created/completed, status, credits, actions.Bug Fixes
sourceFilein Convert Again to avoid lazy loading and N+1.Written for commit 05bf0a1. Summary will update on new commits.
Summary by CodeRabbit
Release Notes