fix: respect replaceState/keepFocus/noScroll when a navigation results in a redirect (#14424)

* respect replace_state on external navigation where applicable

* changeset

* added tests

* fixed client navigation

* prettier

* changeset

* take out trash

* take out trash

* take out trash

* Apply suggestion from @eltigerchino

* changeset

* fix typo

* Apply suggestion from @eltigerchino

* Update .changeset/dirty-rats-divide.md

* Delete packages/kit/test/apps/basics/src/routes/navigation/nonexistent/.gitkeep

* add replaceState to invalidate

* fix test

* move tests to `goto` suite

* change tests according to code review suggestions

* take out trash

* test fix

* fix lint

* fix typo

* typedef

* typedef

* fix lint

* refactor test

* Revert "fix lint"

This reverts commit 481e1ab.

* remove opts from invalidate/All

* always set `replaceState` to `true` when a load function throws a redirect while invalidating

* call navigate directly and forward other state as well

* sort imports like on main

* we don't need an `opts` object on `native_navigation`

* revert invalidate in tests

* revert invalidate/All types

* changelog

* 20

---------

Co-authored-by: akonstantinov <[email protected]>
Co-authored-by: Tee Ming <[email protected]>

Author: 3 people

.changeset/dirty-rats-divide.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+fix: respect `replaceState`/`keepFocus`/`noScroll` when a navigation results in a redirect

packages/kit/src/runtime/client/client.js

@@ -149,9 +149,14 @@ function clear_onward_history(current_history_index, current_navigation_index) {
  * Returns a `Promise` that never resolves (to prevent any
  * subsequent work, e.g. history manipulation, from happening)
  * @param {URL} url
+ * @param {boolean} [replace] If `true`, will replace the current `history` entry rather than creating a new one with `pushState`
  */
-function native_navigation(url) {
-	location.href = url.href;
+function native_navigation(url, replace = false) {
+	if (replace) {
+		location.replace(url.href);
+	} else {
+		location.href = url.href;
+	}
 	return new Promise(() => {});
 }
 
@@ -388,7 +393,12 @@ async function _invalidate(include_load_functions = true, reset_page_state = tru
 		if (!navigation_result || nav_token !== token) return;
 
 		if (navigation_result.type === 'redirect') {
-			return _goto(new URL(navigation_result.location, current.url).href, {}, 1, nav_token);
+			return _goto(
+				new URL(navigation_result.location, current.url).href,
+				{ replaceState: true },
+				1,
+				nav_token
+			);
 		}
 
 		// This is a bit hacky but allows us not having to pass that boolean around, making things harder to reason about
@@ -1532,10 +1542,11 @@ async function navigate({
 							route: { id: null }
 						}
 					),
-					404
+					404,
+					replace_state
 				);
 			} else {
-				return await native_navigation(url);
+				return await native_navigation(url, replace_state);
 			}
 		} else {
 			navigation_result = await server_fallback(
@@ -1546,7 +1557,8 @@ async function navigate({
 					params: {},
 					route: { id: null }
 				}),
-				404
+				404,
+				replace_state
 			);
 		}
 	}
@@ -1563,27 +1575,36 @@ async function navigate({
 
 	if (navigation_result.type === 'redirect') {
 		// whatwg fetch spec https://fetch.spec.whatwg.org/#http-redirect-fetch says to error after 20 redirects
-		if (redirect_count >= 20) {
-			navigation_result = await load_root_error_page({
-				status: 500,
-				error: await handle_error(new Error('Redirect loop'), {
-					url,
-					params: {},
-					route: { id: null }
-				}),
-				url,
-				route: { id: null }
+		if (redirect_count < 20) {
+			return navigate({
+				type,
+				url: new URL(navigation_result.location, url),
+				popped,
+				keepfocus,
+				noscroll,
+				replace_state,
+				state,
+				redirect_count: redirect_count + 1,
+				nav_token
 			});
-		} else {
-			await _goto(new URL(navigation_result.location, url).href, {}, redirect_count + 1, nav_token);
-			return false;
 		}
+
+		navigation_result = await load_root_error_page({
+			status: 500,
+			error: await handle_error(new Error('Redirect loop'), {
+				url,
+				params: {},
+				route: { id: null }
+			}),
+			url,
+			route: { id: null }
+		});
 	} else if (/** @type {number} */ (navigation_result.props.page.status) >= 400) {
 		const updated = await stores.updated.check();
 		if (updated) {
 			// Before reloading, try to update the service worker if it exists
 			await update_service_worker();
-			await native_navigation(url);
+			await native_navigation(url, replace_state);
 		}
 	}
 
@@ -1725,9 +1746,10 @@ async function navigate({
  * @param {{ id: string | null }} route
  * @param {App.Error} error
  * @param {number} status
+ * @param {boolean} [replace_state]
  * @returns {Promise<import('./types.js').NavigationFinished>}
  */
-async function server_fallback(url, route, error, status) {
+async function server_fallback(url, route, error, status, replace_state) {
 	if (url.origin === origin && url.pathname === location.pathname && !hydrated) {
 		// We would reload the same page we're currently on, which isn't hydrated,
 		// which means no SSR, which means we would end up in an endless loop
@@ -1747,7 +1769,7 @@ async function server_fallback(url, route, error, status) {
 		debugger; // eslint-disable-line
 	}
 
-	return await native_navigation(url);
+	return await native_navigation(url, replace_state);
 }
 
 if (import.meta.hot) {

packages/kit/test/apps/basics/src/routes/goto/loadreplace1/+page.ts

@@ -0,0 +1,5 @@
+import { redirect } from '@sveltejs/kit';
+
+export const load = () => {
+	return redirect(302, '/goto/loadreplace2');
+};

packages/kit/test/apps/basics/src/routes/goto/loadreplace2/+page.ts

@@ -0,0 +1,5 @@
+import { redirect } from '@sveltejs/kit';
+
+export const load = () => {
+	return redirect(302, '/goto/loadreplace3');
+};

packages/kit/test/apps/basics/src/routes/goto/loadreplace3/+page.ts

@@ -0,0 +1,5 @@
+import { redirect } from '@sveltejs/kit';
+
+export const load = () => {
+	return redirect(302, '/goto/testfinish');
+};

packages/kit/test/apps/basics/src/routes/goto/testentry/+page.svelte

@@ -0,0 +1 @@
+<h3>navigation test entry</h3>

packages/kit/test/apps/basics/src/routes/goto/testfinish/+page.svelte

@@ -0,0 +1 @@
+<h3>navigation test finish</h3>

packages/kit/test/apps/basics/src/routes/goto/teststart/+page.js

@@ -0,0 +1,8 @@
+import { redirect } from '@sveltejs/kit';
+
+export const load = ({ depends, url, untrack }) => {
+	depends('app:goto');
+	if (untrack(() => url.searchParams.get('redirect') !== null)) {
+		return redirect(302, '/goto/loadreplace1');
+	}
+};

packages/kit/test/apps/basics/src/routes/goto/teststart/+page.svelte

@@ -0,0 +1 @@
+<h3>navigation test start</h3>

packages/kit/test/apps/basics/test/client.test.js

@@ -1364,6 +1364,130 @@ test.describe('goto', () => {
 			: 'goto: invalid URL';
 		await expect(page.locator('p')).toHaveText(message);
 	});
+
+	test.describe('navigation and redirects should be consistent between web native and sveltekit based', () => {
+		const testEntryPage = '/goto/testentry';
+		const testStartPage = '/goto/teststart';
+		const testFinishPage = '/goto/testfinish';
+		const nonexistentPage = '/goto/nonexistent';
+		const loadReplacePage = '/goto/loadreplace1';
+
+		test.beforeEach(async ({ page }) => {
+			await page.goto(testEntryPage);
+			await page.goto(testStartPage);
+
+			await expect(page).toHaveURL(testStartPage);
+		});
+
+		/**
+		 * @param {string} from
+		 * @param {string} to
+		 * @returns {(page: import('@playwright/test').Page) => Promise<void>}
+		 */
+		function makeExpectGoback(from, to) {
+			return async (page) => {
+				await expect(page).toHaveURL(from);
+				await page.goBack();
+				await expect(page).toHaveURL(to);
+			};
+		}
+
+		test.describe('navigating outside the app on sameorigin', () => {
+			test.describe('without replace', () => {
+				const expectGoback = makeExpectGoback(nonexistentPage, testStartPage);
+
+				test('app.goto', async ({ app, page }) => {
+					// navigating to nonexistent page causes playwright's page context to be destroyed
+					// thus this call throws an error unless caught
+					await app.goto(nonexistentPage, { replaceState: false }).catch(() => {});
+					await expectGoback(page);
+				});
+
+				test('location.assign', async ({ page }) => {
+					await page.evaluate((url) => {
+						location.assign(url);
+					}, nonexistentPage);
+					await expectGoback(page);
+				});
+			});
+
+			test.describe('with replace', () => {
+				const expectGoback = makeExpectGoback(nonexistentPage, testEntryPage);
+
+				test('app.goto', async ({ app, page }) => {
+					// navigating to nonexistent page causes playwright's page context to be destroyed
+					// thus this call throws an error unless caught
+					await app.goto(nonexistentPage, { replaceState: true }).catch(() => {});
+					await expectGoback(page);
+				});
+
+				test('location.replace', async ({ page }) => {
+					await page.evaluate((url) => {
+						location.replace(url);
+					}, nonexistentPage);
+					await expectGoback(page);
+				});
+			});
+		});
+
+		test.describe('redirect after invalidation', () => {
+			test.beforeEach(async ({ app }) => {
+				await app.goto(`${testStartPage}?redirect`, { replaceState: true });
+			});
+
+			const expectGoback = makeExpectGoback(testFinishPage, testEntryPage);
+
+			test('app.invalidate', async ({ app, page }) => {
+				await app.invalidate('app:goto', { replaceState: true });
+				await expectGoback(page);
+			});
+
+			test('location.reload', async ({ page }) => {
+				await page.evaluate(() => {
+					location.reload();
+				});
+				await expectGoback(page);
+			});
+		});
+
+		test.describe('navigating through redirect chain', () => {
+			test.describe('without replace', () => {
+				const expectGoback = makeExpectGoback(testFinishPage, testStartPage);
+
+				test('app.goto', async ({ app, page }) => {
+					await app.goto(loadReplacePage, { replaceState: false });
+
+					await expectGoback(page);
+				});
+
+				test('location.assign', async ({ page }) => {
+					await page.evaluate((url) => {
+						location.assign(url);
+					}, loadReplacePage);
+
+					await expectGoback(page);
+				});
+			});
+
+			test.describe('with replace', () => {
+				const expectGoback = makeExpectGoback(testFinishPage, testEntryPage);
+
+				test('app.goto', async ({ app, page }) => {
+					await app.goto(loadReplacePage, { replaceState: true });
+
+					await expectGoback(page);
+				});
+
+				test('location.replace', async ({ page }) => {
+					await page.evaluate((url) => {
+						location.replace(url);
+					}, loadReplacePage);
+
+					await expectGoback(page);
+				});
+			});
+		});
+	});
 });
 
 test.describe('untrack', () => {