Monthly Shift: July 2026 - #503
Conversation
📝 WalkthroughWalkthroughThis PR performs a codebase-wide dependency normalization: replacing ChangesImport normalization and guard-clause refactor
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase by standardizing Carbon imports to Illuminate\Support\Carbon, cleaning up facade imports, and replacing standard conditional exception/abort blocks with Laravel's throw_if, throw_unless, abort_if, and abort_unless helpers. However, the widespread use of throw_if and throw_unless with eagerly instantiated exception objects (and associated string formatting) introduces noticeable performance overhead and unnecessary memory allocation on successful execution paths. It is highly recommended to either revert these to standard if statements or pass the exception class name and constructor arguments to the helpers to enable lazy instantiation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| throw_if(abs($originalTotal - $newTotal) > self::AMOUNT_COMPARISON_EPSILON, new RuntimeException( | ||
| sprintf( | ||
| 'Transaction item merge aborted: amount mismatch for transaction %d (original %.4f vs new %.4f).', | ||
| $transaction->id, | ||
| $originalTotal, | ||
| $newTotal, | ||
| ) | ||
| )); |
There was a problem hiding this comment.
Using throw_if here introduces a significant performance regression. In PHP, arguments to functions are evaluated eagerly. This means sprintf() is executed and a new RuntimeException object (including its expensive stack trace generation) is instantiated on every single successful merge operation, even when there is no amount mismatch.
It is highly recommended to revert this change to the original if statement so that the string formatting and exception instantiation only occur in the exceptional case when a mismatch actually happens.
if (abs($originalTotal - $newTotal) > self::AMOUNT_COMPARISON_EPSILON) {
throw new RuntimeException(
sprintf(
'Transaction item merge aborted: amount mismatch for transaction %d (original %.4f vs new %.4f).',
$transaction->id,
$originalTotal,
$newTotal,
)
);
}| throw_if(isset($data->{'Error Message'}), new InvalidPriceDataException( | ||
| "Alpha Vantage API error: {$data->{'Error Message'}}", | ||
| 'alpha_vantage', | ||
| $investment->symbol | ||
| )); | ||
|
|
||
| if (isset($data->Note)) { | ||
| throw new PriceProviderException( | ||
| "Alpha Vantage rate limit: {$data->Note}", | ||
| 'alpha_vantage', | ||
| $investment->symbol | ||
| ); | ||
| } | ||
| throw_if(isset($data->Note), new PriceProviderException( | ||
| "Alpha Vantage rate limit: {$data->Note}", | ||
| 'alpha_vantage', | ||
| $investment->symbol | ||
| )); |
There was a problem hiding this comment.
Inside this block, if Time Series (Daily) is missing but neither Error Message nor Note is present in the response, the code will eagerly instantiate both InvalidPriceDataException and PriceProviderException before finally throwing the fallback InvalidPriceDataException on line 64.
Using throw_if here is highly inefficient and confusing. It is much cleaner and more performant to use standard if statements or to pass the exception class names and arguments to throw_if to avoid eager instantiation. Reverting to standard if statements is recommended here for readability and performance.
if (isset($data->{'Error Message'})) {
throw new InvalidPriceDataException(
"Alpha Vantage API error: {$data->{'Error Message'}}",
'alpha_vantage',
$investment->symbol
);
}
if (isset($data->Note)) {
throw new PriceProviderException(
"Alpha Vantage rate limit: {$data->Note}",
'alpha_vantage',
$investment->symbol
);
}| if ($accountEntity !== null && $accountEntity->user_id !== $user->id) { | ||
| throw new AuthorizationException('You do not have permission to access this account entity.'); | ||
| } | ||
| throw_if($accountEntity !== null && $accountEntity->user_id !== $user->id, new AuthorizationException('You do not have permission to access this account entity.')); |
There was a problem hiding this comment.
Using throw_if with an instantiated exception object (new AuthorizationException(...)) causes the exception to be eagerly instantiated on every single request, even when the user is fully authorized. Generating exception stack traces is a relatively expensive operation in PHP.
To prevent this performance overhead, you can pass the exception class name as a string and the message as a subsequent argument. The throw_if helper will then lazily instantiate the exception only when the condition is met.
throw_if($accountEntity !== null && $accountEntity->user_id !== $user->id, AuthorizationException::class, 'You do not have permission to access this account entity.');| throw_if(! is_string($providerKey) || $providerKey === '', new PriceProviderException( | ||
| 'Investment has no price provider configured', | ||
| 'none', | ||
| $investment->symbol | ||
| )); | ||
|
|
||
| throw_unless($this->providerRegistry->has($providerKey), new PriceProviderException( | ||
| "Investment has unknown provider: {$providerKey}", | ||
| $providerKey, | ||
| $investment->symbol | ||
| )); |
There was a problem hiding this comment.
Using throw_if and throw_unless with eagerly instantiated exception objects (new PriceProviderException(...)) causes these exceptions to be created on every single resolution call, even when the provider key is valid and registered.
To avoid this overhead, pass the exception class name as a string and the constructor arguments as subsequent parameters so that the exception is only instantiated when the condition is met.
throw_if(! is_string($providerKey) || $providerKey === '', PriceProviderException::class, 'Investment has no price provider configured', 'none', $investment->symbol);
throw_unless($this->providerRegistry->has($providerKey), PriceProviderException::class, "Investment has unknown provider: {$providerKey}", $providerKey, $investment->symbol);![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
♻️ Duplicate comments (1)
app/Services/InvestmentPriceProviders/AlphaVantageProvider.php (1)
148-156: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame eager-exception-construction issue as
fetchPrices().
throw_if(isset($data->{'Error Message'}), new PriceProviderException("...{$data->{'Error Message'}}"...))and theNoteguard below it unconditionally construct the exception (and interpolate the property) even when the property is absent, emitting undefined-property warnings on the normal/success path. Same fix as above: revert these two guards toif (isset(...)) { throw ... }.🤖 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/InvestmentPriceProviders/AlphaVantageProvider.php` around lines 148 - 156, The two Alpha Vantage error guards in AlphaVantageProvider are eagerly constructing PriceProviderException and interpolating properties even on the success path, which can trigger undefined-property warnings. Update the error-handling block in the provider to use explicit if (isset(...)) checks for both the "Error Message" and Note cases, and only instantiate and throw the exception inside those branches, matching the safer pattern used in fetchPrices().
🧹 Nitpick comments (2)
database/migrations/2026_04_02_173148_backfill_web_scraping_provider_settings_on_investments_table.php (1)
45-50: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMinor: exception object built on every iteration, even on success.
throw_if($encodedSettings === false, new RuntimeException(sprintf(...)))evaluates thesprintfandjson_last_error_msg()call on every changed row regardless of whether encoding actually failed, since the exception argument is constructed eagerly beforethrow_ifchecks the condition. Impact is negligible here (bounded chunk size, one-time migration), but worth noting for readers unfamiliar with the eager-evaluation gotcha ofthrow_if/throw_unlesshelpers.♻️ Optional refactor to avoid eager construction
- $encodedSettings = json_encode($settings); - throw_if($encodedSettings === false, new RuntimeException(sprintf( - 'Failed to encode provider_settings for investment id %d: %s', - (int) $investment->id, - json_last_error_msg() - ))); + $encodedSettings = json_encode($settings); + + if ($encodedSettings === false) { + throw new RuntimeException(sprintf( + 'Failed to encode provider_settings for investment id %d: %s', + (int) $investment->id, + json_last_error_msg() + )); + }🤖 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_04_02_173148_backfill_web_scraping_provider_settings_on_investments_table.php` around lines 45 - 50, The migration’s provider_settings encoding check eagerly builds the RuntimeException for every investment even when json_encode succeeds. Update the logic in the backfill migration’s encoding block to only construct the exception inside the failure path (for example, an explicit conditional around the json_encode result) so the expensive sprintf/json_last_error_msg work is deferred until needed.app/Http/Traits/ScheduleTrait.php (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse word-style logical operators per project convention.
Line 31 uses symbolic
&&/!instead of the project's preferredand/not. Logic itself is correct and matches existing call sites.As per coding guidelines, "Use logical operators (and, or, not) instead of symbolic operators (&&, ||, !) in PHP control structures."
✏️ Suggested style fix
- throw_if($startType === 'custom' && ! $customStart, new InvalidArgumentException('Custom start date is required for custom start type')); + throw_if($startType === 'custom' and not $customStart, new InvalidArgumentException('Custom start date is required for custom start type'));🤖 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/Traits/ScheduleTrait.php` around lines 29 - 31, The conditional in ScheduleTrait’s throw_if check uses symbolic operators instead of the project’s preferred word-style logical operators. Update the custom start validation in ScheduleTrait so the existing logic is expressed with and/not rather than &&/!, keeping the same throw_if behavior and InvalidArgumentException message.Source: Coding guidelines
🤖 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/Services/InvestmentPriceProviders/AlphaVantageProvider.php`:
- Around line 52-62: Replace the two `throw_if()` calls in
`AlphaVantageProvider::getPriceData` with explicit `if` guards so the exception
objects are only constructed when the response actually contains `Error Message`
or `Note`. Access `{$data->{'Error Message'}}` and `{$data->Note}` only inside
those conditionals, and throw `InvalidPriceDataException` or
`PriceProviderException` directly from the guarded blocks to avoid
undefined-property warnings and unnecessary exception creation.
In `@app/Services/OcrService.php`:
- Line 124: The OCR process failure check in OcrService::run should keep the
explicit success check and throw path instead of using throw_unless, because the
ProcessFailedException is being constructed eagerly and can fail even when the
process succeeds. Replace the throw_unless usage with a normal conditional
around the $process->isSuccessful() check, and only instantiate
ProcessFailedException with $process when the process is actually unsuccessful.
---
Duplicate comments:
In `@app/Services/InvestmentPriceProviders/AlphaVantageProvider.php`:
- Around line 148-156: The two Alpha Vantage error guards in
AlphaVantageProvider are eagerly constructing PriceProviderException and
interpolating properties even on the success path, which can trigger
undefined-property warnings. Update the error-handling block in the provider to
use explicit if (isset(...)) checks for both the "Error Message" and Note cases,
and only instantiate and throw the exception inside those branches, matching the
safer pattern used in fetchPrices().
---
Nitpick comments:
In `@app/Http/Traits/ScheduleTrait.php`:
- Around line 29-31: The conditional in ScheduleTrait’s throw_if check uses
symbolic operators instead of the project’s preferred word-style logical
operators. Update the custom start validation in ScheduleTrait so the existing
logic is expressed with and/not rather than &&/!, keeping the same throw_if
behavior and InvalidArgumentException message.
In
`@database/migrations/2026_04_02_173148_backfill_web_scraping_provider_settings_on_investments_table.php`:
- Around line 45-50: The migration’s provider_settings encoding check eagerly
builds the RuntimeException for every investment even when json_encode succeeds.
Update the logic in the backfill migration’s encoding block to only construct
the exception inside the failure path (for example, an explicit conditional
around the json_encode result) so the expensive sprintf/json_last_error_msg work
is deferred until needed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fbcd371-d022-404f-b1b2-85a519166f59
📒 Files selected for processing (70)
app/Console/Commands/GetInvestmentPrices.phpapp/Console/Commands/ProcessAiDocuments.phpapp/Console/Commands/RecordScheduledTransactions.phpapp/Console/Commands/ResetDemoDatabase.phpapp/Contracts/InvestmentPriceProvider.phpapp/Http/Controllers/API/AccountApiController.phpapp/Http/Controllers/API/AiDocumentApiController.phpapp/Http/Controllers/API/AiProviderConfigApiController.phpapp/Http/Controllers/API/CategoryLearningApiController.phpapp/Http/Controllers/API/InvestmentApiController.phpapp/Http/Controllers/API/InvestmentPriceApiController.phpapp/Http/Controllers/API/InvestmentPriceProviderApiController.phpapp/Http/Controllers/API/ReportApiController.phpapp/Http/Controllers/AccountEntityController.phpapp/Http/Controllers/AiDocumentController.phpapp/Http/Controllers/TransactionController.phpapp/Http/Traits/CurrencyTrait.phpapp/Http/Traits/ScheduleTrait.phpapp/Jobs/AiProcessingJob.phpapp/Jobs/CalculateAccountMonthlySummary.phpapp/Listeners/CreateAiDocumentFromSource.phpapp/Listeners/ProcessTransactionUpdated.phpapp/Models/Account.phpapp/Models/AccountGroup.phpapp/Models/AccountMonthlySummary.phpapp/Models/Category.phpapp/Models/Currency.phpapp/Models/Transaction.phpapp/Services/AiStepGateway.phpapp/Services/CurrencyRateService.phpapp/Services/DuplicateDetectionService.phpapp/Services/ImagePreprocessingService.phpapp/Services/InvestmentPriceProviderContextResolver.phpapp/Services/InvestmentPriceProviderRegistry.phpapp/Services/InvestmentPriceProviders/AlphaVantageProvider.phpapp/Services/InvestmentPriceProviders/GenericApiProvider.phpapp/Services/InvestmentPriceProviders/WebScrapingProvider.phpapp/Services/InvestmentService.phpapp/Services/OcrService.phpapp/Services/ProcessDocumentService.phpapp/Services/TextExtractionService.phpapp/Services/TransactionItemMergeService.phpdatabase/migrations/2026_01_31_000001_add_transaction_type_enum_column_to_transactions_table.phpdatabase/migrations/2026_01_31_000002_add_unsigned_to_decimal_columns.phpdatabase/migrations/2026_04_02_173148_backfill_web_scraping_provider_settings_on_investments_table.phptests/Browser/Pages/Transactions/TransactionShowInvestmentModalTest.phptests/Feature/API/V1/InvestmentPriceProviderApiV1Test.phptests/Feature/AiProviderConfigApiControllerTest.phptests/Feature/CategoryTest.phptests/Feature/Console/GetInvestmentPricesCommandTest.phptests/Feature/Console/RecordScheduledTransactionsCommandTest.phptests/Feature/CurrencyRateTest.phptests/Feature/GoogleDriveConfigApiControllerTest.phptests/Feature/PayeeStatsApiControllerTest.phptests/Feature/ProcessGoogleDriveConfigJobTest.phptests/Unit/Console/Commands/RecordScheduledTransactionsTest.phptests/Unit/Http/Controllers/API/PayeeApiControllerTest.phptests/Unit/Http/Controllers/API/PayeeStatsApiControllerTest.phptests/Unit/Jobs/CalculateAccountMonthlySummaryTest.phptests/Unit/Models/AiProviderConfigTest.phptests/Unit/Models/CurrencyTest.phptests/Unit/Models/GoogleDriveConfigTest.phptests/Unit/Models/InvestmentProviderConfigTest.phptests/Unit/Models/TransactionScheduleTest.phptests/Unit/Services/DuplicateDetectionServiceTest.phptests/Unit/Services/InvestmentPriceProviders/AlphaVantageProviderTest.phptests/Unit/Services/InvestmentPriceProviders/GenericApiProviderTest.phptests/Unit/Services/InvestmentPriceProviders/WebScrapingProviderTest.phptests/Unit/Services/InvestmentServicePriceTest.phptests/Unit/Services/ProcessDocumentServiceTest.php
| throw_if(isset($data->{'Error Message'}), new InvalidPriceDataException( | ||
| "Alpha Vantage API error: {$data->{'Error Message'}}", | ||
| 'alpha_vantage', | ||
| $investment->symbol | ||
| )); | ||
|
|
||
| if (isset($data->Note)) { | ||
| throw new PriceProviderException( | ||
| "Alpha Vantage rate limit: {$data->Note}", | ||
| 'alpha_vantage', | ||
| $investment->symbol | ||
| ); | ||
| } | ||
| throw_if(isset($data->Note), new PriceProviderException( | ||
| "Alpha Vantage rate limit: {$data->Note}", | ||
| 'alpha_vantage', | ||
| $investment->symbol | ||
| )); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== AlphaVantageProvider excerpt ==\n'
sed -n '1,140p' app/Services/InvestmentPriceProviders/AlphaVantageProvider.php | cat -n
printf '\n== throw_if definition search ==\n'
rg -n "function throw_if|throw_if\(" -S app vendor bootstrap config --glob '!vendor/composer/*' || true
printf '\n== PHP eager evaluation probe ==\n'
php -d display_errors=1 -d error_reporting=E_ALL <<'PHP'
<?php
class E extends Exception {}
$obj = (object) [];
function probe($cond, $ex) {
echo "probe called\n";
}
echo "1) direct interpolation in arguments\n";
try {
probe(isset($obj->{'Error Message'}), new E("msg: {$obj->{'Error Message'}}"));
} catch (Throwable $t) {
echo get_class($t), ": ", $t->getMessage(), "\n";
}
echo "2) guarded if\n";
try {
if (isset($obj->{'Error Message'})) {
throw new E("msg: {$obj->{'Error Message'}}");
}
echo "no warning\n";
} catch (Throwable $t) {
echo get_class($t), ": ", $t->getMessage(), "\n";
}
PHPRepository: kantorge/yaffa
Length of output: 13360
Use if guards here instead of throw_if with prebuilt exceptions. The exception arguments are evaluated before throw_if() runs, so {$data->{'Error Message'}} and {$data->Note} are accessed even when those fields are missing, which raises undefined-property warnings on the common “missing Time Series” path. Reverting these two branches to conditional throws avoids the warning and the extra exception construction.
🤖 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/InvestmentPriceProviders/AlphaVantageProvider.php` around lines
52 - 62, Replace the two `throw_if()` calls in
`AlphaVantageProvider::getPriceData` with explicit `if` guards so the exception
objects are only constructed when the response actually contains `Error Message`
or `Note`. Access `{$data->{'Error Message'}}` and `{$data->Note}` only inside
those conditionals, and throw `InvalidPriceDataException` or
`PriceProviderException` directly from the guarded blocks to avoid
undefined-property warnings and unnecessary exception creation.
| if (! $process->isSuccessful()) { | ||
| throw new ProcessFailedException($process); | ||
| } | ||
| throw_unless($process->isSuccessful(), new ProcessFailedException($process)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Keep the explicit if/throw here instead of throw_unless. new ProcessFailedException($process) is evaluated eagerly and can throw for a successful process.
🤖 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/OcrService.php` at line 124, The OCR process failure check in
OcrService::run should keep the explicit success check and throw path instead of
using throw_unless, because the ProcessFailedException is being constructed
eagerly and can fail even when the process succeeds. Replace the throw_unless
usage with a normal conditional around the $process->isSuccessful() check, and
only instantiate ProcessFailedException with $process when the process is
actually unsuccessful.
2 participants
This is an automated pull request included with your Shifty Plan. It contains curated refactors to keep your Laravel application aligned with the latest conventions and features.
This month focuses on a set of curated refactors to keep your Laravel apps modernized and your code streamlined. These are a subset of the refactors performed by the Laravel Fixer, which is included with your subscription.
Before merging, you should:
shift-2026-07branchIf you do not wish to adopt these refactors, you may simply close this pull request and delete its branch.
Summary by CodeRabbit
Bug Fixes
Refactor