Repository navigation
Feature/490 errorhandler refactor - #491
Merged
armanist merged 5 commits intoMay 1, 2026
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughErrorHandler is refactored into an orchestrator that delegates severity resolution, stack-trace formatting, and web rendering to new collaborators: ExceptionSeverityResolver, StackTraceFormatter, and WebExceptionRenderer; CLI/web handling and logging behavior adjusted, and rendering fallbacks added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ErrorHandler
participant ExceptionSeverityResolver
participant StackTraceFormatter
participant WebExceptionRenderer
participant FileSystem
participant ViewFactory
Client->>ErrorHandler: Exception occurs
ErrorHandler->>ExceptionSeverityResolver: resolve(throwable)
ExceptionSeverityResolver-->>ErrorHandler: severity
alt CLI Mode
ErrorHandler-->>Client: print message (or verbose diagnostics if debug)
else Web Mode
ErrorHandler->>StackTraceFormatter: compose(throwable)
StackTraceFormatter->>FileSystem: read source lines
FileSystem-->>StackTraceFormatter: source snippet
StackTraceFormatter-->>ErrorHandler: formatted trace
ErrorHandler->>WebExceptionRenderer: render(throwable, severity)
WebExceptionRenderer->>ViewFactory: renderPartial(trace or 500)
ViewFactory-->>WebExceptionRenderer: HTML
alt render success
WebExceptionRenderer-->>ErrorHandler: rendered HTML
else render throws
WebExceptionRenderer-->>ErrorHandler: exception
ErrorHandler-->>ErrorHandler: catch & produce fallback 500 response
end
ErrorHandler->>ErrorHandler: log(severity) if non-debug
ErrorHandler-->>Client: send HTTP 500 with body
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 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. Review rate limit: 0/1 reviews remaining, refill in 42 minutes and 36 seconds.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #491 +/- ##
============================================
+ Coverage 90.71% 90.73% +0.01%
- Complexity 2905 2912 +7
============================================
Files 249 252 +3
Lines 7655 7670 +15
============================================
+ Hits 6944 6959 +15
Misses 711 711 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Tracer/ErrorHandler.php (1)
115-126:⚠️ Potential issue | 🟠 MajorExplicitly set HTTP 500 status in the fallback path.
Line 125 calls
response()->html($errorPage)without setting a status code, which defaults to 200 OK. The emergency fallback should explicitly return HTTP 500.Pass the status code as the second parameter:
response()->html($errorPage, StatusCode::INTERNAL_SERVER_ERROR)or chainresponse()->setStatusCode(500)->html($errorPage).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Tracer/ErrorHandler.php` around lines 115 - 126, The fallback path currently renders the error page with response()->html($errorPage) which leaves the status as 200; update the emergency catch/fallback in Tracer/ErrorHandler.php so the response uses HTTP 500—e.g. call response()->html($errorPage, StatusCode::INTERNAL_SERVER_ERROR) or chain response()->setStatusCode(500)->html($errorPage) before response()->send(); keep the existing log via $this->logError($throwable, $errorType) and is_debug_mode() logic intact.
🧹 Nitpick comments (1)
tests/Unit/Tracer/ErrorHandlerTest.php (1)
159-175: ⚡ Quick winStrengthen fallback test to assert HTTP contract, not only body text.
This test currently validates content only. Please also assert the fallback is returned as a 500 plain-text response so the acceptance criterion is regression-proof.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Tracer/ErrorHandlerTest.php` around lines 159 - 175, Update testHandleWebExceptionFallsBackWhenRendererThrows to assert the full HTTP fallback contract: after invoking ErrorHandler::handleWebException (via the test's invokePrivateMethodOn on $handler), in addition to asserting response()->getContent() equals 'Internal Server Error', also assert response()->getStatusCode() returns 500 and response()->headers->get('Content-Type') (or response()->headers->get('content-type')) equals a plain-text MIME like 'text/plain' (or starts with 'text/plain') so the test verifies status, body and content-type when WebExceptionRenderer::render throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Tracer/ErrorHandler.php`:
- Around line 115-126: The fallback path currently renders the error page with
response()->html($errorPage) which leaves the status as 200; update the
emergency catch/fallback in Tracer/ErrorHandler.php so the response uses HTTP
500—e.g. call response()->html($errorPage, StatusCode::INTERNAL_SERVER_ERROR) or
chain response()->setStatusCode(500)->html($errorPage) before
response()->send(); keep the existing log via $this->logError($throwable,
$errorType) and is_debug_mode() logic intact.
---
Nitpick comments:
In `@tests/Unit/Tracer/ErrorHandlerTest.php`:
- Around line 159-175: Update testHandleWebExceptionFallsBackWhenRendererThrows
to assert the full HTTP fallback contract: after invoking
ErrorHandler::handleWebException (via the test's invokePrivateMethodOn on
$handler), in addition to asserting response()->getContent() equals 'Internal
Server Error', also assert response()->getStatusCode() returns 500 and
response()->headers->get('Content-Type') (or
response()->headers->get('content-type')) equals a plain-text MIME like
'text/plain' (or starts with 'text/plain') so the test verifies status, body and
content-type when WebExceptionRenderer::render throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bf79040-3b9c-4f92-88e5-3a24a70b162f
📒 Files selected for processing (8)
src/Tracer/ErrorHandler.phpsrc/Tracer/ExceptionSeverityResolver.phpsrc/Tracer/StackTraceFormatter.phpsrc/Tracer/WebExceptionRenderer.phptests/Unit/Tracer/ErrorHandlerTest.phptests/Unit/Tracer/ExceptionSeverityResolverTest.phptests/Unit/Tracer/StackTraceFormatterTest.phptests/Unit/Tracer/WebExceptionRendererTest.php
andrey-smaelov
requested changes
May 1, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Tracer/ErrorHandler.php`:
- Around line 112-123: handleWebException currently calls
$this->webExceptionRenderer->render($throwable) and response()->send() directly,
which can themselves throw and prevent a guaranteed error response; wrap the
render + response()->html(...) and response()->send() calls in a
try/catch(Throwable $e) block inside handleWebException so that if rendering
fails you log the rendering failure (via $this->logError or similar) and then
emit a plain-text 500 response (response()->text("Internal Server Error",
StatusCode::INTERNAL_SERVER_ERROR) or equivalent) and call response()->send();
keep the existing logging of the original $throwable (the existing
$this->logError($throwable, $errorType) call) intact and only suppress
additional debug rendering when is_debug_mode() is false.
In `@tests/Unit/Tracer/ErrorHandlerTest.php`:
- Around line 158-177: The test currently expects the WebExceptionRenderer's
thrown Exception to propagate, but the ErrorHandler::handleWebException must
instead fall back to emitting a minimal plain-text 500 response; update
testHandleWebExceptionPropagatesRendererFailure to remove the
expectException/expectExceptionMessage calls and instead call
$this->invokePrivateMethod($handler, 'handleWebException', [new
Exception('boom')]) inside the ob_start/try block, capture the output via $body
= ob_get_clean() and assert that no exception was thrown and that $body equals
(or contains) the minimal plain-text fallback message and that the handler
produced a 500 response outcome (i.e. assert fallback content and 500 status
semantics) referencing the test method name and ErrorHandler::handleWebException
and WebExceptionRenderer::render to locate the code to change.
🪄 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: c7eb88f5-8e22-415a-9f33-ac694f873b4d
📒 Files selected for processing (5)
src/Tracer/ErrorHandler.phptests/Unit/AppTestCase.phptests/Unit/Tracer/ErrorHandlerTest.phptests/Unit/Tracer/StackTraceFormatterTest.phptests/Unit/Tracer/WebExceptionRendererTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/Unit/Tracer/WebExceptionRendererTest.php
andrey-smaelov
approved these changes
May 1, 2026
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
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.
Closes #490
Summary by CodeRabbit
New Features
Improvements
Tests