From 787cfbeba38231ad0efde05a0b42b74cfbe4d857 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Thu, 16 Nov 2023 11:29:51 +0000
Subject: [PATCH] Fix followRedirects when source is async and destination is
 sync

The previous implementation of `followRedirects()` would catch a transition rejection and check the router for an `activeTransition`. This can become problematic in async situations because the destination transition may have resolved before the `reject` is scheduled on the previous transition. One such case is when redirecting from an async model hook to a destination route with synchronous model hooks.

This commit updates the `followRedirects()` logic to explicitly follow the redirect chain rather than relying on the presence of an `activeTransition`. This makes following redirects work correctly regardless of any scheduling concerns.

This problem has been noted in the context of the `visit()` test helper:

- https://github.com/emberjs/ember-test-helpers/issues/332
- https://github.com/emberjs/ember.js/issues/17150
---
 lib/router/transition.ts | 10 ++++++----
 tests/router_test.ts     | 29 +++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/lib/router/transition.ts b/lib/router/transition.ts
index 3a43749..88be275 100644
--- a/lib/router/transition.ts
+++ b/lib/router/transition.ts
@@ -28,6 +28,7 @@ export type OpaqueTransition = PublicTransition;
 export const STATE_SYMBOL = `__STATE__-2619860001345920-3322w3`;
 export const PARAMS_SYMBOL = `__PARAMS__-261986232992830203-23323`;
 export const QUERY_PARAMS_SYMBOL = `__QPS__-2619863929824844-32323`;
+export const REDIRECT_DESTINATION_SYMBOL = `__RDS__-2619863929824844-32323`;
 
 /**
   A Transition is a thenable (a promise-like object) that represents
@@ -71,6 +72,7 @@ export default class Transition<R extends Route> implements Partial<Promise<R>>
   isCausedByAbortingReplaceTransition = false;
   _visibleQueryParams: Dict<unknown> = {};
   isIntermediate = false;
+  [REDIRECT_DESTINATION_SYMBOL]?: Transition<R>;
 
   /**
     In non-production builds, this function will return the stack that this Transition was
@@ -309,6 +311,7 @@ export default class Transition<R extends Route> implements Partial<Promise<R>>
   }
 
   redirect(newTransition: Transition<R>) {
+    this[REDIRECT_DESTINATION_SYMBOL] = newTransition;
     this.rollback();
     this.router.routeWillChange(newTransition);
   }
@@ -419,10 +422,9 @@ export default class Transition<R extends Route> implements Partial<Promise<R>>
     @public
    */
   followRedirects(): Promise<R> {
-    let router = this.router;
-    return this.promise!.catch(function (reason) {
-      if (router.activeTransition) {
-        return router.activeTransition.followRedirects();
+    return this.promise!.catch((reason) => {
+      if (this[REDIRECT_DESTINATION_SYMBOL]) {
+        return this[REDIRECT_DESTINATION_SYMBOL]!.followRedirects();
       }
       return Promise.reject(reason);
     });
diff --git a/tests/router_test.ts b/tests/router_test.ts
index 44340ba..bc28934 100644
--- a/tests/router_test.ts
+++ b/tests/router_test.ts
@@ -4682,6 +4682,35 @@ scenarios.forEach(function (scenario) {
       });
   });
 
+  test('Transition#followRedirects() works correctly when redirecting from an async model hook', function (assert) {
+    assert.expect(2);
+
+    routes.index = createHandler('index', {
+      beforeModel: function () {
+        return Promise.resolve(true).then(() => {
+          return router.transitionTo('about');
+        });
+      },
+    });
+
+    routes.about = createHandler('about', {
+      setup: function () {
+        assert.ok(true, 'about#setup was called');
+      },
+    });
+
+    router
+      .transitionTo('/index')
+      .followRedirects()
+      .then(function (handler: Route) {
+        assert.equal(
+          handler,
+          routes.about,
+          'followRedirects works with redirect from async hook transitions'
+        );
+      });
+  });
+
   test("Returning a redirecting Transition from a model hook doesn't cause things to explode", function (assert) {
     assert.expect(2);