Conversation
📝 WalkthroughWalkthroughThe changes extend error reporting in app/worker.php to include previous exception details when chained exceptions occur. Two error handling paths are modified: the logError function now logs previous exception message (when different from current), file, and line as extras; the executor error handler similarly outputs previous exception information to console. Existing behavior is preserved when no previous exception is present. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/worker.php (1)
497-530: Worker error handler lacks previous exception tracking.The
logErrorfunction (lines 414-420, 442-447) now tracks previous exception details, but this worker error handler does not. For consistent error reporting across all error handling paths, consider adding the same previous exception tracking here.After line 512, add previous exception tracking to the log:
$previousError = $error->getPrevious(); if ($previousError !== null) { if ($previousError->getMessage() !== $error->getMessage()) { $log->addExtra('previousMessage', $previousError->getMessage()); } $log->addExtra('previousFile', $previousError->getFile()); $log->addExtra('previousLine', $previousError->getLine()); $log->addExtra('previousTrace', $previousError->getTraceAsString()); }After line 529, add previous exception console output:
$previousError = $error->getPrevious(); if ($previousError !== null) { if ($previousError->getMessage() !== $error->getMessage()) { Console::error('[Error] Previous Message: ' . $previousError->getMessage()); } Console::error('[Error] Previous File: ' . $previousError->getFile()); Console::error('[Error] Previous Line: ' . $previousError->getLine()); }
🧹 Nitpick comments (2)
app/worker.php (2)
414-420: Consider cachinggetPrevious()and including previous exception trace.The previous exception is accessed multiple times. Consider caching it for clarity and minor efficiency gains. Additionally, including the previous exception's trace would provide more complete debugging context for chained exceptions.
Apply this diff to cache the result and add trace information:
- if ($error->getPrevious() !== null) { - if ($error->getPrevious()->getMessage() != $error->getMessage()) { - $log->addExtra('previousMessage', $error->getPrevious()->getMessage()); + $previousError = $error->getPrevious(); + if ($previousError !== null) { + if ($previousError->getMessage() !== $error->getMessage()) { + $log->addExtra('previousMessage', $previousError->getMessage()); } - $log->addExtra('previousFile', $error->getPrevious()->getFile()); - $log->addExtra('previousLine', $error->getPrevious()->getLine()); + $log->addExtra('previousFile', $previousError->getFile()); + $log->addExtra('previousLine', $previousError->getLine()); + $log->addExtra('previousTrace', $previousError->getTraceAsString()); }
442-447: CachegetPrevious()for console output as well.Similar to the logging section above, the previous exception is accessed multiple times here. Apply the same caching pattern for consistency and clarity.
Apply this diff:
- if ($error->getPrevious() !== null) { - if ($error->getPrevious()->getMessage() != $error->getMessage()) { - Console::warning("Previous Failed: {$error->getPrevious()->getMessage()}"); + $previousError = $error->getPrevious(); + if ($previousError !== null) { + if ($previousError->getMessage() !== $error->getMessage()) { + Console::warning("Previous Failed: {$previousError->getMessage()}"); } - Console::warning("Previous File: {$error->getPrevious()->getFile()} Line: {$error->getPrevious()->getLine()}"); + Console::warning("Previous File: {$previousError->getFile()} Line: {$previousError->getLine()}"); + Console::warning($previousError->getTraceAsString()); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/worker.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
✨ Benchmark results
⚡ Benchmark Comparison
|
…ker-error-add-previous
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist