Improved error handeling - #1094
Conversation
WalkthroughErrorsController was updated to use a shared Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router
participant ErrorsController
participant ViewTemplate
Client->>Router: request to error route (/404,/406,/422,/500)
Router->>ErrorsController: dispatch to specific error action
ErrorsController->>ViewTemplate: render 'errors/<action>' with layout 'errors' and status
ViewTemplate-->>ErrorsController: rendered HTML
ErrorsController-->>Client: HTTP response (error page)
Note over ErrorsController: render_error_page rescues StandardError\nlogs error and renders plain-text fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #1094 +/- ##
===========================================
- Coverage 75.15% 74.56% -0.60%
===========================================
Files 53 51 -2
Lines 1103 1093 -10
===========================================
- Hits 829 815 -14
- Misses 274 278 +4 ☔ 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.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/errors_controller.rb(1 hunks)public/404.html(0 hunks)public/406-unsupported-browser.html(0 hunks)public/422.html(0 hunks)public/500.html(0 hunks)
💤 Files with no reviewable changes (4)
- public/500.html
- public/404.html
- public/406-unsupported-browser.html
- public/422.html
⏰ 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). (2)
- GitHub Check: Test
- GitHub Check: Lint
🔇 Additional comments (1)
app/controllers/errors_controller.rb (1)
5-17: All error view templates are present and correct.The verification confirms that all four templates referenced in
app/controllers/errors_controller.rbexist atapp/views/errors/:
forbidden.html.erbnot_found.html.erbunacceptable.html.erbinternal_server_error.html.erbThe code is ready for deployment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/views/errors/unprocessable_entity.html.erb (1)
1-2: Consider a more user-friendly heading.The heading "unprocessable_entity (422)" is very technical. Since the rest of the page uses friendly Dutch text, consider using a more user-friendly heading like "Verzoek niet verwerkt" or "Ongeldige gegevens" while keeping the error code for debugging purposes.
Apply this diff for a friendlier heading:
-<% content_for :title, "unprocessable_entity (422) - #{Rails.application.config.x.site_name}" %> -<h1>unprocessable_entity (422)</h1> +<% content_for :title, "Verzoek niet verwerkt (422) - #{Rails.application.config.x.site_name}" %> +<h1>Verzoek niet verwerkt <small>(422)</small></h1>app/controllers/errors_controller.rb (1)
4-37: Consider extracting rescue logic to reduce duplication.All five error actions share nearly identical rescue blocks, differing only in the error message and status code. This duplication could be reduced with a helper method or by using a more generic rescue approach.
Consider extracting the common pattern:
class ErrorsController < ApplicationController layout 'errors' rescue_from StandardError, with: :handle_rendering_error def forbidden render_error_page('errors/forbidden', :forbidden, '403 Forbidden') end def not_found render_error_page('errors/not_found', :not_found, '404 Not Found') end def unacceptable render_error_page('errors/unacceptable', :not_acceptable, '406 Not Acceptable') end def unprocessable_entity render_error_page('errors/unprocessable_entity', :unprocessable_entity, '422 Unprocessable Entity') end def internal_server_error render_error_page('errors/internal_server_error', :internal_server_error, '500 Internal Server Error') end private def render_error_page(template, status, fallback_message) render template: template, status: status rescue StandardError => e Rails.logger.error("Error rendering #{status} page: #{e.message}") render plain: fallback_message, status: status end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/errors_controller.rb(1 hunks)app/views/errors/unprocessable_entity.html.erb(1 hunks)config/routes.rb(1 hunks)
🔇 Additional comments (4)
app/views/errors/unprocessable_entity.html.erb (1)
3-4: LGTM - Clear user guidance.The Dutch error message and contact information are clear and helpful. The mailto link with admin email configuration is implemented correctly.
config/routes.rb (1)
68-69: LGTM - Error routes correctly mapped.The routes now correctly map 406 to the
unacceptableaction and 422 to the newunprocessable_entityaction. This separation ensures proper HTTP status codes are returned for each error type.app/controllers/errors_controller.rb (2)
2-2: LGTM - Error-specific layout prevents cascading failures.Using the 'errors' layout instead of 'application' is the correct approach to prevent the bootloop issue mentioned in the PR description. This ensures error pages can render even when the application is in a degraded state.
25-30: LGTM - New unprocessable_entity action follows established pattern.The new action correctly renders the 422 template with appropriate error handling and fallback, consistent with the other error actions.
1 participant
The staging env was targeted with a wordpress penetration attack and now ended up in a bootlood due to bad error handeling
Summary by CodeRabbit
New Features
Bug Fixes
Refactor