Project: Better frontend error handling


(Kane York) #1

With my laptop and the spotty campus WiFi, I’ve noticed that Discourse doesn’t handle sudden connectivity loss very well. For example, try this test:

  • Disconnect network
  • Click notifications bubble

Expect: Some visual indication that I clicked on the bubble. Anything!
Actual: Uncaught promise is logged to the console and no header popup is shown.

  • Disconnect network
  • Click logo

Expect: Some sort of graceful degradation
Actual: Infinite loading spinner


Additionally, it would be nice if the backend had cleaner paths to error returning. If I look at application_controller.rb, I see at least 4 utility functions for returning an error:

  • handle_unverified_request returns a 403 with the text ['BAD CSRF']. This is handled by Discourse.ajax().
  • rescuing from RateLimiter::LimitExceeded returns 429 with a JSON object with an errors array and a time_left parameter.
  • rescuing from Discourse::NotLoggedIn either redirects to / or returns a 403 with failed_json.merge, adding a message string.
  • Similarly, but a little differently, rescuing from Discourse::ReadOnly returns a 405 with failed_json merged with a message.
  • Discourse::NotFound and Discourse::InvalidAccess call rescue_discourse_actions, which has such weird behavior, I’m going to need subbullets to explain it:
    • If the request is from the URL bar or an external link, the ‘Not Found’ page with a login button is returned. (example)
    • If the request is from JS, but the status is 404, the Not Found page is returned anyways.
    • If the request is from JS and the status isn’t 404, it renders plaintext: either [error: 'not found'] or [error: 'invalid access']. Both of these, and the Not Found page, are guaranteed to break the JSON parser.

Other controllers use the failed_json.merge method, but there’s even another error path: render_json_error which is called with an ActiveRecord object and serializes its .errors.full_messages. admin/screened_ip_addresses_controller provides a nice example of this here.

Just as an extra punch in the face, if you’re running in development mode, none of the rescue_froms are run and instead you get the beautiful Rails error page: Discourse Meta
(Oh, this also happens for the ActiveRecord errors too, so it’s also impossible to do manual tests of render_json_error in a development environment.)


I’ve started trying to add all the catches here.

What it does is repurpose the render_json_error method from something specific to ActiveRecord objects to taking any array of messages (that’s the return type of full_messages). It gives the client a JSON array like this:

{
  success: false, /* compat with success_json */
  failed: 'FAILED', /* compat with failed_json */
  messages: ["The site is in read only mode. Interactions are disabled."]
}

An intermediate goal would be to make sure that every Discourse.ajax() call either provides an error handler or passes the promise to a function that adds one. The status-complete state would be that every call has an appropriate response to the request failing.


(Kane York) #2

@eviltrout you should know that lib/mention.js's lookup method is a Zalgo API: blog.izs.me — Designing APIs for Asynchrony

This will become horribly apparent if you try to add a working catch to the Discourse.ajax call.


(Robin Ward) #3

That is some of our oldest and cruftiest code. I will make a note to refactor it, hopefully this week!


(Robin Ward) #4

Cool I fixed the zalgo API. There seemed to be a lot of unused code in there. I removed the unnecessary bits and converted it to a promise.

https://github.com/discourse/discourse/commit/ed6e2b1d79ed8ef7fef675876ba781a37deb6576


(Régis Hanol) #5


(Kane York) #6

Progress.

@eviltrout @codinghorror

This is set up in application_route's error action. And yes, Try Again actually works.


(Jeff Atwood) #7

Looks really good, please carry on!