Release v0.1.14 — Phase 14: Custom Credit Ledger#183
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 35 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 (8)
📝 WalkthroughWalkthroughThis PR introduces a complete user credit system with database-backed ledger operations, automatic account provisioning during user registration, and dashboard integration. It adds contract-based credit operations (grant, spend, refund), transaction tracking with metadata support, and a UI component displaying available credits. ChangesUser Credit System
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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
app/Observers/UserObserver.php (1)
17-19: ⚡ Quick win
monthly_creditsis enforced as numeric by plan config +PlanLimitcasting
config/feature-access.phpdefinesmonthly_creditsas numbers forfree/pro/max(50/1000/5000), andapp/Services/FeatureAccess/PlanLimit.php::fromArray()requires themonthly_creditskey and casts it viamonthlyCredits: (int) $limits['monthly_credits']. That makes non-numeric sentinel values from plan limits unlikely to reachUserObserverand cause the$amount > 0grant to be skipped silently. The(int)cast inapp/Observers/UserObserver.phpis therefore redundant in the current code 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 `@app/Observers/UserObserver.php` around lines 17 - 19, Remove the redundant (int) cast on $amount in UserObserver.php: rely on FeatureAccessService::limit($user, 'monthly_credits') (which is backed by PlanLimit::fromArray() and config/feature-access.php) to provide a numeric value; update the $amount assignment to use the service return directly and leave the existing conditional if ($amount > 0) intact so behavior remains identical while avoiding unnecessary casting.database/migrations/2026_06_02_110332_create_credit_transactions_table.php (1)
14-28: 💤 Low valueConsider adding an index on
expires_atif expiry queries are common.The schema includes an
expires_atcolumn, suggesting support for expiring credits. If the application needs to query for expired credits (e.g., a scheduled job that finds and processes all expired transactions), an index onexpires_atwould improve performance. If expiry is only checked on a per-user basis during balance calculation, the existing(user_id, created_at)index may suffice.🔍 Optional index addition
$table->index(['user_id', 'created_at']); $table->index(['user_id', 'type']); + $table->index('expires_at'); });🤖 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/migrations/2026_06_02_110332_create_credit_transactions_table.php` around lines 14 - 28, Add an index on the expires_at column in the credit_transactions table to optimize expiry queries: inside the Schema::create callback (the credit_transactions migration that defines columns like id, user_id, amount, expires_at), add an index for 'expires_at' (e.g., via $table->index('expires_at')) alongside the existing $table->index(['user_id', 'created_at']) and $table->index(['user_id', 'type']) to improve performance for scheduled/expiry lookups.app/Contracts/Billing/CreditLedger.php (1)
11-20: ⚡ Quick winConsider documenting expected exceptions in the contract.
The interface defines mutation operations but doesn't document what exceptions implementers should throw. Based on the PR summary, implementations should throw
InsufficientCreditsExceptionandInvalidCreditAmountException, but callers have no way to discover this from the contract alone. Consider adding PHPDoc blocks that declare@throwsannotations for each method.📝 Suggested PHPDoc additions
interface CreditLedger { + /** + * Get the current credit balance for a user. + * + * `@param` User $user + * `@return` int + */ public function balance(User $user): int; + /** + * Grant credits to a user. + * + * `@param` User $user + * `@param` int $amount + * `@param` string $reason + * `@param` array $meta + * `@param` Model|null $source + * `@return` CreditTransaction + * `@throws` \App\Exceptions\Billing\InvalidCreditAmountException + */ public function grant(User $user, int $amount, string $reason, array $meta = [], ?Model $source = null): CreditTransaction; + /** + * Spend credits from a user's account. + * + * `@param` User $user + * `@param` int $amount + * `@param` string $reason + * `@param` array $meta + * `@param` Model|null $source + * `@return` CreditTransaction + * `@throws` \App\Exceptions\Billing\InsufficientCreditsException + * `@throws` \App\Exceptions\Billing\InvalidCreditAmountException + */ public function spend(User $user, int $amount, string $reason, array $meta = [], ?Model $source = null): CreditTransaction; + /** + * Refund credits to a user's account. + * + * `@param` User $user + * `@param` int $amount + * `@param` string $reason + * `@param` array $meta + * `@param` Model|null $source + * `@return` CreditTransaction + * `@throws` \App\Exceptions\Billing\InvalidCreditAmountException + */ public function refund(User $user, int $amount, string $reason, array $meta = [], ?Model $source = null): CreditTransaction; }🤖 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/Contracts/Billing/CreditLedger.php` around lines 11 - 20, Add PHPDoc `@throws` annotations to the CreditLedger interface to document expected exceptions for implementers and callers: on the mutation methods grant, spend and refund (in interface CreditLedger) add `@throws` \App\Exceptions\Billing\InvalidCreditAmountException and `@throws` \App\Exceptions\Billing\InsufficientCreditsException (spend/refund must at least document InsufficientCreditsException; grant should document InvalidCreditAmountException), leaving balance unchanged; use fully-qualified exception class names so callers can discover the contract.app/Services/Billing/DatabaseCreditLedger.php (2)
21-21: 💤 Low valueRedundant type cast.
The
(int)cast is unnecessary since theCreditAccountmodel already castsbalanceto integer (line 22 inapp/Models/CreditAccount.php).♻️ Proposed simplification
- return (int) $user->creditAccount()->firstOrCreate(['user_id' => $user->id], ['balance' => 0])->balance; + return $user->creditAccount()->firstOrCreate(['user_id' => $user->id], ['balance' => 0])->balance;🤖 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/Services/Billing/DatabaseCreditLedger.php` at line 21, Remove the redundant (int) cast in DatabaseCreditLedger.php: change the return expression that currently prepends (int) to the CreditAccount balance to rely on the CreditAccount model's attribute cast; update the return in the method that calls $user->creditAccount()->firstOrCreate([...], ['balance' => 0])->balance so it returns the already-casted integer from the model (no other logic changes needed).
24-50: ⚡ Quick winConsider extracting shared logic between grant() and refund().
The
grant()andrefund()methods share nearly identical implementations (both increment balance, usefirstOrCreate, same transaction structure). Extracting common logic into a private helper method would reduce duplication and improve maintainability.♻️ Example refactoring
+ private function addCredits( + User $user, + int $amount, + CreditTransactionType $type, + string $reason, + array $meta = [], + ?Model $source = null + ): CreditTransaction { + return DB::transaction(function () use ($user, $amount, $type, $reason, $meta, $source) { + if ($amount <= 0) { + throw InvalidCreditAmountException::becauseAmountMustBePositive(); + } + + $account = CreditAccount::query() + ->where('user_id', $user->id) + ->lockForUpdate() + ->firstOrCreate(['user_id' => $user->id], ['balance' => 0]); + + $account->increment('balance', $amount); + $account->refresh(); + + return CreditTransaction::create([ + 'user_id' => $user->id, + 'amount' => $amount, + 'balance_after' => $account->balance, + 'type' => $type, + 'reason' => $reason, + 'metadata_json' => $meta ?: null, + 'source_type' => $source?->getMorphClass(), + 'source_id' => $source?->getKey(), + ]); + }); + } + public function grant(User $user, int $amount, string $reason, array $meta = [], ?Model $source = null): CreditTransaction { - return DB::transaction(function () use ($user, $amount, $reason, $meta, $source) { - if ($amount <= 0) { - throw InvalidCreditAmountException::becauseAmountMustBePositive(); - } - - $account = CreditAccount::query() - ->where('user_id', $user->id) - ->lockForUpdate() - ->firstOrCreate(['user_id' => $user->id], ['balance' => 0]); - - $account->increment('balance', $amount); - $account->refresh(); - - return CreditTransaction::create([ - 'user_id' => $user->id, - 'amount' => $amount, - 'balance_after' => $account->balance, - 'type' => CreditTransactionType::Grant, - 'reason' => $reason, - 'metadata_json' => $meta ?: null, - 'source_type' => $source?->getMorphClass(), - 'source_id' => $source?->getKey(), - ]); - }); + return $this->addCredits($user, $amount, CreditTransactionType::Grant, $reason, $meta, $source); } public function refund(User $user, int $amount, string $reason, array $meta = [], ?Model $source = null): CreditTransaction { - return DB::transaction(function () use ($user, $amount, $reason, $meta, $source) { - if ($amount <= 0) { - throw InvalidCreditAmountException::becauseAmountMustBePositive(); - } - - $account = CreditAccount::query() - ->where('user_id', $user->id) - ->lockForUpdate() - ->firstOrCreate(['user_id' => $user->id], ['balance' => 0]); - - $account->increment('balance', $amount); - $account->refresh(); - - return CreditTransaction::create([ - 'user_id' => $user->id, - 'amount' => $amount, - 'balance_after' => $account->balance, - 'type' => CreditTransactionType::Refund, - 'reason' => $reason, - 'metadata_json' => $meta ?: null, - 'source_type' => $source?->getMorphClass(), - 'source_id' => $source?->getKey(), - ]); - }); + return $this->addCredits($user, $amount, CreditTransactionType::Refund, $reason, $meta, $source); }Also applies to: 84-110
🤖 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/Services/Billing/DatabaseCreditLedger.php` around lines 24 - 50, Both grant() and refund() duplicate the DB transaction, account lookup/lock/firstOrCreate, balance change, refresh and CreditTransaction::create logic; extract that into a private helper (e.g., adjustBalance(User $user, int $delta, CreditTransactionType $type, string $reason, array $meta = [], ?Model $source = null)) that performs the validation (throw InvalidCreditAmountException when appropriate), runs the DB::transaction, uses CreditAccount::query()->where(...)->lockForUpdate()->firstOrCreate(...), calls increment('balance', $delta) or decrement as needed, refreshes the model and returns the created CreditTransaction; then have grant() and refund() call this helper with the correct delta and CreditTransactionType and remove the duplicated code.tests/Feature/Credits/RegistrationCreditsTest.php (1)
24-33: ⚡ Quick winStrengthen the "no duplicate grant" assertion.
Checking only the final balance is an indirect proxy. The test would still pass if an update accidentally created a second
registration_granttransaction that was somehow netted out, or if balance reconciliation hid it. Asserting theregistration_granttransaction count is exactly 1 directly verifies the no-duplication intent.♻️ Proposed addition
expect(app(CreditLedger::class)->balance($user))->toBe($expected); + + expect( + \App\Models\CreditTransaction::query() + ->where('user_id', $user->id) + ->where('reason', 'registration_grant') + ->count() + )->toBe(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/Credits/RegistrationCreditsTest.php` around lines 24 - 33, The test should explicitly assert that only one registration grant transaction exists for the user instead of only checking the final balance; after creating and updating the $user, add an assertion that the count of CreditTransaction records with type 'registration_grant' for $user (e.g. CreditTransaction::where('type','registration_grant')->where('user_id',$user->id)->count()) equals 1, while keeping the existing balance assertion that uses app(CreditLedger::class)->balance($user) and FeatureAccessService::limit($user,'monthly_credits').tests/Unit/Enums/CreditTransactionTypeTest.php (1)
7-14: ⚡ Quick winConsider verifying enum completeness to catch future changes.
The test correctly verifies each enum value, but it doesn't ensure these are all the cases. If a new case is added to
CreditTransactionTypeor an existing one is removed, this test won't detect the addition (it will silently pass with incomplete coverage).✨ Suggested improvement for completeness check
it('defines credit transaction types', function () { + $cases = CreditTransactionType::cases(); + expect($cases)->toHaveCount(6); + expect(CreditTransactionType::Grant->value)->toBe('grant'); expect(CreditTransactionType::Purchase->value)->toBe('purchase'); expect(CreditTransactionType::Spend->value)->toBe('spend'); expect(CreditTransactionType::Refund->value)->toBe('refund'); expect(CreditTransactionType::Adjustment->value)->toBe('adjustment'); expect(CreditTransactionType::Expiration->value)->toBe('expiration'); });Alternatively, for a more robust approach that verifies both count and values:
it('defines credit transaction types', function () { - expect(CreditTransactionType::Grant->value)->toBe('grant'); - expect(CreditTransactionType::Purchase->value)->toBe('purchase'); - expect(CreditTransactionType::Spend->value)->toBe('spend'); - expect(CreditTransactionType::Refund->value)->toBe('refund'); - expect(CreditTransactionType::Adjustment->value)->toBe('adjustment'); - expect(CreditTransactionType::Expiration->value)->toBe('expiration'); + $expected = [ + 'Grant' => 'grant', + 'Purchase' => 'purchase', + 'Spend' => 'spend', + 'Refund' => 'refund', + 'Adjustment' => 'adjustment', + 'Expiration' => 'expiration', + ]; + + $cases = CreditTransactionType::cases(); + expect($cases)->toHaveCount(count($expected)); + + foreach ($expected as $name => $value) { + expect(CreditTransactionType::{$name}->value)->toBe($value); + } });🤖 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/Unit/Enums/CreditTransactionTypeTest.php` around lines 7 - 14, The test only asserts individual values but doesn't ensure no extra enum cases exist; update the test in CreditTransactionTypeTest (the it(...) block) to assert completeness by comparing CreditTransactionType::cases() (or array_map(fn($c) => $c->value, CreditTransactionType::cases())) against an explicit expected array of values and also assert the count matches, so additions/removals will fail the test.
🤖 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/Models/CreditTransaction.php`:
- Around line 19-29: In the CreditTransaction model remove the calculated
snapshot field from mass-assignable attributes: delete 'balance_after' from the
protected $fillable array in class CreditTransaction so it cannot be set via
mass assignment; if the ledger service still needs to set it, update usages to
assign via direct property + save (e.g. $transaction->balance_after = $value;
$transaction->save();) or use forceFill when necessary instead of leaving
'balance_after' in $fillable.
In `@tests/Feature/Credits/CreditAccountTest.php`:
- Around line 33-37: The test "it('new user credit account is created with
balance matching starter grant')" currently only asserts
$user->creditAccount->balance >= 0 which doesn't match the test name; update the
assertion to check the exact starter grant value instead (e.g. compare
$user->creditAccount->balance to the configured starter/monthly credits used
elsewhere such as the monthly_credits value referenced in
RegistrationCreditsTest), by modifying the expectation on
$user->creditAccount->balance in this test so it equals the canonical starter
grant value rather than just being non‑negative.
---
Nitpick comments:
In `@app/Contracts/Billing/CreditLedger.php`:
- Around line 11-20: Add PHPDoc `@throws` annotations to the CreditLedger
interface to document expected exceptions for implementers and callers: on the
mutation methods grant, spend and refund (in interface CreditLedger) add `@throws`
\App\Exceptions\Billing\InvalidCreditAmountException and `@throws`
\App\Exceptions\Billing\InsufficientCreditsException (spend/refund must at least
document InsufficientCreditsException; grant should document
InvalidCreditAmountException), leaving balance unchanged; use fully-qualified
exception class names so callers can discover the contract.
In `@app/Observers/UserObserver.php`:
- Around line 17-19: Remove the redundant (int) cast on $amount in
UserObserver.php: rely on FeatureAccessService::limit($user, 'monthly_credits')
(which is backed by PlanLimit::fromArray() and config/feature-access.php) to
provide a numeric value; update the $amount assignment to use the service return
directly and leave the existing conditional if ($amount > 0) intact so behavior
remains identical while avoiding unnecessary casting.
In `@app/Services/Billing/DatabaseCreditLedger.php`:
- Line 21: Remove the redundant (int) cast in DatabaseCreditLedger.php: change
the return expression that currently prepends (int) to the CreditAccount balance
to rely on the CreditAccount model's attribute cast; update the return in the
method that calls $user->creditAccount()->firstOrCreate([...], ['balance' =>
0])->balance so it returns the already-casted integer from the model (no other
logic changes needed).
- Around line 24-50: Both grant() and refund() duplicate the DB transaction,
account lookup/lock/firstOrCreate, balance change, refresh and
CreditTransaction::create logic; extract that into a private helper (e.g.,
adjustBalance(User $user, int $delta, CreditTransactionType $type, string
$reason, array $meta = [], ?Model $source = null)) that performs the validation
(throw InvalidCreditAmountException when appropriate), runs the DB::transaction,
uses CreditAccount::query()->where(...)->lockForUpdate()->firstOrCreate(...),
calls increment('balance', $delta) or decrement as needed, refreshes the model
and returns the created CreditTransaction; then have grant() and refund() call
this helper with the correct delta and CreditTransactionType and remove the
duplicated code.
In `@database/migrations/2026_06_02_110332_create_credit_transactions_table.php`:
- Around line 14-28: Add an index on the expires_at column in the
credit_transactions table to optimize expiry queries: inside the Schema::create
callback (the credit_transactions migration that defines columns like id,
user_id, amount, expires_at), add an index for 'expires_at' (e.g., via
$table->index('expires_at')) alongside the existing $table->index(['user_id',
'created_at']) and $table->index(['user_id', 'type']) to improve performance for
scheduled/expiry lookups.
In `@tests/Feature/Credits/RegistrationCreditsTest.php`:
- Around line 24-33: The test should explicitly assert that only one
registration grant transaction exists for the user instead of only checking the
final balance; after creating and updating the $user, add an assertion that the
count of CreditTransaction records with type 'registration_grant' for $user
(e.g.
CreditTransaction::where('type','registration_grant')->where('user_id',$user->id)->count())
equals 1, while keeping the existing balance assertion that uses
app(CreditLedger::class)->balance($user) and
FeatureAccessService::limit($user,'monthly_credits').
In `@tests/Unit/Enums/CreditTransactionTypeTest.php`:
- Around line 7-14: The test only asserts individual values but doesn't ensure
no extra enum cases exist; update the test in CreditTransactionTypeTest (the
it(...) block) to assert completeness by comparing
CreditTransactionType::cases() (or array_map(fn($c) => $c->value,
CreditTransactionType::cases())) against an explicit expected array of values
and also assert the count matches, so additions/removals will fail the test.
🪄 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: dc08d963-6f52-48fb-a0e3-bac7aa2ccadd
📒 Files selected for processing (25)
app/Contracts/Billing/CreditLedger.phpapp/Enums/CreditTransactionType.phpapp/Exceptions/Billing/InsufficientCreditsException.phpapp/Exceptions/Billing/InvalidCreditAmountException.phpapp/Models/CreditAccount.phpapp/Models/CreditTransaction.phpapp/Models/User.phpapp/Observers/UserObserver.phpapp/Providers/AppServiceProvider.phpapp/Services/Billing/DatabaseCreditLedger.phpapp/View/Components/UserDropdown.phpdatabase/factories/CreditAccountFactory.phpdatabase/factories/CreditTransactionFactory.phpdatabase/migrations/2026_06_02_110224_create_credit_accounts_table.phpdatabase/migrations/2026_06_02_110332_create_credit_transactions_table.phpresources/views/components/user-dropdown.blade.phptests/Feature/Credits/CreditAccountAutoCreationTest.phptests/Feature/Credits/CreditAccountTest.phptests/Feature/Credits/CreditLedgerTest.phptests/Feature/Credits/CreditTransactionTest.phptests/Feature/Credits/RegistrationCreditsTest.phptests/Feature/Credits/UserDropdownCreditsTest.phptests/Unit/Billing/DatabaseCreditLedgerBindingTest.phptests/Unit/Contracts/CreditLedgerContractTest.phptests/Unit/Enums/CreditTransactionTypeTest.php
- Remove balance_after from CreditTransaction fillable; use forceCreate in ledger - Extract addToBalance() helper in DatabaseCreditLedger (grant/refund dedup) - Remove redundant (int) cast in balance() — model already casts to int - Add @throws annotations to CreditLedger contract - Add expires_at index to credit_transactions migration - Strengthen CreditAccountTest: assert exact starter grant value - Strengthen RegistrationCreditsTest: assert exactly one registration_grant transaction - Strengthen CreditTransactionTypeTest: assert completeness via cases() comparison
There was a problem hiding this comment.
2 issues found across 25 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- CreditAccountFactory: wrap User factory in withoutObservers() to prevent duplicate credit_account insert when the UserObserver auto-creates one - DatabaseCreditLedger::spend(): replace firstOrFail() with firstOrCreate() so an accountless user gets InsufficientCreditsException (balance=0) instead of a ModelNotFoundException
Summary
CreditAccountand starter credits from plan config on registrationWhat's included (CONV-198 – CONV-214)
credit_accountsandcredit_transactionstables with migrationsCreditAccountandCreditTransactionEloquent modelsCreditTransactionTypeenum (Grant / Purchase / Spend / Refund / Adjustment / Expiration)CreditLedgercontract bound toDatabaseCreditLedgerin the containerDatabaseCreditLedger— all mutations wrapped inDB::transactionwithlockForUpdate()on the account rowInsufficientCreditsExceptionandInvalidCreditAmountExceptiondomain exceptionsUserObserver— createsCreditAccountand grants registration starter credits (value fromFeatureAccessService::limit('monthly_credits')) on user creationCreditLedgerTest plan
composer test— 388 tests passcomposer lint— no violationsnpm run build— builds cleanmonthly_creditsInsufficientCreditsException, balance unchanged, no spend transaction created🤖 Generated with Claude Code
Summary by cubic
Introduce a local, database-backed credit ledger with per-user accounts and transactions, independent of any payment system (covers CONV-198–CONV-214). New users get starter credits from plan limits, and the balance appears in the user dropdown; run DB migrations (no Stripe/Cashier needed).
New Features
expires_at).CreditLedgerbound toDatabaseCreditLedger; operations run in DB transactions with row locks.monthly_credits.Bug Fixes
CreditAccountFactory.spend(), usefirstOrCreate()so users without an account get anInsufficientCreditsException(balance=0) instead of a model-not-found error.Written for commit 7e8da4d. Summary will update on new commits.
Summary by CodeRabbit
Release Notes