Skip to content

Handle Task::NotFoundError with a 404 page instead of raising an error#1440

Open
kaibadash wants to merge 1 commit intoShopify:mainfrom
kaibadash:fix/handle404
Open

Handle Task::NotFoundError with a 404 page instead of raising an error#1440
kaibadash wants to merge 1 commit intoShopify:mainfrom
kaibadash:fix/handle404

Conversation

@kaibadash
Copy link
Copy Markdown
Contributor

deleted task
  • Add rescue_from Task::NotFoundError in ApplicationController to render a friendly 404 page when users access a task that no longer exists
  • Create a dedicated not found view (not_found.html.erb) styled with Bulma to match the existing UI, with a link back to the tasks list
  • Remove the rescue_responses mapping from Engine since the error is now handled directly by the controller

Motivation

Maintenance tasks are short-lived by nature — they are created for one-time data migrations or cleanup operations and are expected to be deleted once they are no longer needed. However, users may still have bookmarked URLs or links in Slack/docs pointing to deleted tasks.

Previously, accessing a deleted task with no associated runs raised MaintenanceTasks::Task::NotFoundError, which:

  1. Displayed an unfriendly error page to users
  2. Was reported to error monitoring systems (e.g., Sentry) as an unhandled exception, creating noise

By using rescue_from in the controller, the exception is caught before it propagates to error monitoring middleware, and users see a clean 404 page with navigation back to the task list.

@etiennebarrie
Copy link
Copy Markdown
Member

2. Was reported to error monitoring systems (e.g., Sentry) as an unhandled exception, creating noise

This shouldn't be the case, since we declare NotFoundError as a rescuable exception that results in a 404:

config.action_dispatch.rescue_responses.merge!(
"MaintenanceTasks::Task::NotFoundError" => :not_found,
)

$ bin/production &
$ curl -s -D - http://127.0.0.1:3000/maintenance_tasks/tasks/DoesNotExist | grep 404
HTTP/1.1 404 Not Found
  <title>The page you were looking for doesn't exist (404)</title>
  <!-- This file lives in public/404.html -->

Maybe Sentry's code does still mark those as unhandled exceptions, but then it's a bug/feature in their code.

Copy link
Copy Markdown
Member

@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be nice.

At first I thought that shouldn't be necessary (easy enough to tweak the URL), and the Sentry issue is a problem with that gem or the configuration (although it shouldn't be necessary to add this exception class to list of excluded exceptions, doing so would solve that issue).

But the change is small, the page looks nicer (and also the main app's 404 page might be super custom) and moving away from action_dispatch.rescue_responses means better support for exception monitoring tools which don't respect action_dispatch.report_exception.

test "show a not found Task" do
visit maintenance_tasks_path + "/tasks/Maintenance::DoesNotExist"

assert_title "Not Found"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we could explicitly test the 404 but we can't use page.status_code 😞

However the 404 causes an issue with

assert_empty page.driver.browser.logs.get(:browser)
since the browser reports navigating to the page as an error. We can work around this with

Suggested change
assert_title "Not Found"
assert_title "Not Found"
assert_includes page.driver.browser.logs.get(:browser).sole.message, "404"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants