FEATURE: Improvement to history stack handling on server errors

FEATURE: Improvement to history stack handling on server errors

The exception page is shown before Ember can actually figure out what the final destination URL we’re going to is. This means that the new page is not present in the history stack, so if we attempt to use the history stack to go back, we will actually navigate back by two steps. By instead forcing a navigation to the current URL, we achieve the goal of going “back” with no history mucking.

Unfortunately, the actual URL that was attempted is not available. Additionally, this only works for the on-screen back button and not the browser back.

Additionally, several modernizations of the exception page code were made.

diff --git a/app/assets/javascripts/discourse/app/controllers/exception.js b/app/assets/javascripts/discourse/app/controllers/exception.js
index d4a285b..628f492 100644
--- a/app/assets/javascripts/discourse/app/controllers/exception.js
+++ b/app/assets/javascripts/discourse/app/controllers/exception.js
@@ -1,5 +1,6 @@
 import { alias, equal, gte, none } from "@ember/object/computed";
 import discourseComputed, { on } from "discourse-common/utils/decorators";
+import DiscourseURL from "discourse/lib/url";
 import Controller from "@ember/controller";
 import I18n from "I18n";
 import { schedule } from "@ember/runloop";
@@ -31,15 +32,15 @@ export default Controller.extend({
   thrown: null,
   lastTransition: null,
 
-  @discourseComputed
-  isNetwork() {
+  @discourseComputed("thrown")
+  isNetwork(thrown) {
     // never made it on the wire
-    if (this.get("thrown.readyState") === 0) {
+    if (thrown && thrown.readyState === 0) {
       return true;
     }
 
     // timed out
-    if (this.get("thrown.jqTextStatus") === "timeout") {
+    if (thrown && thrown.jqTextStatus === "timeout") {
       return true;
     }
 
@@ -65,16 +66,18 @@ export default Controller.extend({
     this.set("loading", false);
   },
 
-  @discourseComputed("isNetwork", "isServer", "isUnknown")
-  reason() {
-    if (this.isNetwork) {
+  @discourseComputed("isNetwork", "thrown.status", "thrown")
+  reason(isNetwork, thrownStatus, thrown) {
+    if (isNetwork) {
       return I18n.t("errors.reasons.network");
-    } else if (this.isServer) {
+    } else if (thrownStatus >= 500) {
       return I18n.t("errors.reasons.server");
-    } else if (this.isNotFound) {
+    } else if (thrownStatus === 404) {
       return I18n.t("errors.reasons.not_found");
-    } else if (this.isForbidden) {
+    } else if (thrownStatus === 403) {
       return I18n.t("errors.reasons.forbidden");
+    } else if (thrown === null) {
+      return I18n.t("errors.reasons.unknown");
     } else {
       // TODO
       return I18n.t("errors.reasons.unknown");
@@ -83,30 +86,42 @@ export default Controller.extend({
 
   requestUrl: alias("thrown.requestedUrl"),
 
-  @discourseComputed("networkFixed", "isNetwork", "isServer", "isUnknown")
-  desc() {
-    if (this.networkFixed) {
+  @discourseComputed(
+    "networkFixed",
+    "isNetwork",
+    "thrown.status",
+    "thrown.statusText",
+    "thrown"
+  )
+  desc(networkFixed, isNetwork, thrownStatus, thrownStatusText, thrown) {
+    if (networkFixed) {
       return I18n.t("errors.desc.network_fixed");
-    } else if (this.isNetwork) {
+    } else if (isNetwork) {
       return I18n.t("errors.desc.network");
-    } else if (this.isNotFound) {
+    } else if (thrownStatus === 404) {
       return I18n.t("errors.desc.not_found");
-    } else if (this.isServer) {
+    } else if (thrownStatus === 403) {
+      return I18n.t("errors.desc.forbidden");
+    } else if (thrownStatus >= 500) {
       return I18n.t("errors.desc.server", {
-        status: this.get("thrown.status") + " " + this.get("thrown.statusText"),
+        status: thrownStatus + " " + thrownStatusText,
       });
+    } else if (thrown === null) {
+      return I18n.t("errors.desc.unknown");
     } else {
       // TODO
       return I18n.t("errors.desc.unknown");
     }
   },
 
-  @discourseComputed("networkFixed", "isNetwork", "isServer", "isUnknown")
-  enabledButtons() {
-    if (this.networkFixed) {
+  @discourseComputed("networkFixed", "isNetwork", "lastTransition")
+  enabledButtons(networkFixed, isNetwork, lastTransition) {
+    if (networkFixed) {
       return [ButtonLoadPage];
-    } else if (this.isNetwork) {
+    } else if (isNetwork) {
       return [ButtonBackDim, ButtonTryAgain];
+    } else if (!lastTransition) {
+      return [ButtonBackBright];
     } else {
       return [ButtonBackBright, ButtonTryAgain];
     }
@@ -114,14 +129,25 @@ export default Controller.extend({
 
   actions: {
     back() {
-      window.history.back();
+      // Strip off subfolder
+      const currentURL = DiscourseURL.router.location.getURL();
+      if (this.lastTransition && currentURL !== "/exception") {
+        this.lastTransition.abort();
+        this.setProperties({ lastTransition: null, thrown: null });
+        // Can't use routeTo because it handles navigation to the same page
+        DiscourseURL.handleURL(currentURL);
+      } else {
+        window.history.back();
+      }
     },
 
     tryLoading() {
       this.set("loading", true);
 
       schedule("afterRender", () => {
-        this.lastTransition.retry();
+        const transition = this.lastTransition;
+        this.setProperties({ lastTransition: null, thrown: null });
+        transition.retry();
         this.set("loading", false);
       });
     },
diff --git a/app/assets/javascripts/discourse/app/lib/url.js b/app/assets/javascripts/discourse/app/lib/url.js
index c412b6b..441d6b8 100644
--- a/app/assets/javascripts/discourse/app/lib/url.js
+++ b/app/assets/javascripts/discourse/app/lib/url.js
@@ -483,6 +483,7 @@ const DiscourseURL = EmberObject.extend({
 
     transition._discourse_intercepted = true;
     transition._discourse_anchor = elementId;
+    transition._discourse_original_url = path;
 
     const promise = transition.promise || transition;
     promise.then(() => jumpToElement(elementId));
diff --git a/app/assets/javascripts/discourse/app/routes/application.js b/app/assets/javascripts/discourse/app/routes/application.js
index dc2f265..9099fff 100644
--- a/app/assets/javascripts/discourse/app/routes/application.js
+++ b/app/assets/javascripts/discourse/app/routes/application.js
@@ -94,13 +94,7 @@ const ApplicationRoute = DiscourseRoute.extend(OpenComposer, {
     },
 
     error(err, transition) {
-      let xhr = {};
-      if (err.jqXHR) {
-        xhr = err.jqXHR;
-      }
-
-      const xhrOrErr = err.jqXHR ? xhr : err;
-
+      const xhrOrErr = err.jqXHR ? err.jqXHR : err;
       const exceptionController = this.controllerFor("exception");
 
       const c = window.console;
diff --git a/app/assets/javascripts/discourse/app/templates/exception.hbs b/app/assets/javascripts/discourse/app/templates/exception.hbs
index 41555d7..7e3ef72 100644
--- a/app/assets/javascripts/discourse/app/templates/exception.hbs
+++ b/app/assets/javascripts/discourse/app/templates/exception.hbs
@@ -5,9 +5,11 @@
     <div class="error-page">
       <div class="face">:(</div>
       <div class="reason">{{reason}}</div>
-      <div class="url">
-        {{i18n "errors.prev_page"}} <a href={{requestUrl}} data-auto-route="true">{{requestUrl}}</a>
-      </div>
+      {{#if requestUrl}}
+        <div class="url">
+          {{i18n "errors.prev_page"}} <a href={{requestUrl}} data-auto-route="true">{{requestUrl}}</a>
+        </div>
+      {{/if}}
       <div class="desc">
         {{#if networkFixed}}
           {{d-icon "check-circle"}}
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index e7fcbdc..ca9796b 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -299,7 +299,7 @@ class ApplicationController < ActionController::Base
       with_resolved_locale(check_current_user: false) do
         # Include error in HTML format for topics#show.
         if (request.params[:controller] == 'topics' && request.params[:action] == 'show') || (request.params[:controller] == 'categories' && request.params[:action] == 'find_by_slug')
-          opts[:extras] = { html: build_not_found_page(error_page_opts) }
+          opts[:extras] = { html: build_not_found_page(error_page_opts), group: error_page_opts[:group] }
         end
       end
 

GitHub sha: c72bf1d7321f3daae8edfd9897a84d39f0ef1020

This commit appears in #13378 which was approved by eviltrout. It was merged by riking.