refactor(desktop): loop-proof route redirects and contain content crashes

apps/desktop/package.json

@@ -247,11 +247,14 @@
 		"zustand": "5.0.12"
 	},
 	"devDependencies": {
+		"@happy-dom/global-registrator": "20.0.2",
 		"@sentry/vite-plugin": "5.4.0",
 		"@superset/typescript": "workspace:*",
 		"@tailwindcss/vite": "4.2.2",
 		"@tanstack/router-cli": "1.167.17",
 		"@tanstack/router-plugin": "1.168.18",
+		"@testing-library/dom": "10.4.1",
+		"@testing-library/react": "16.3.0",
 		"@types/better-sqlite3": "7.6.13",
 		"@types/bun": "1.3.14",
 		"@types/culori": "4.0.1",

apps/desktop/src/renderer/components/Redirect/Redirect.test.tsx

@@ -0,0 +1,165 @@
+import { afterAll, afterEach, describe, expect, test } from "bun:test";
+import { GlobalRegistrator } from "@happy-dom/global-registrator";
+
+// happy-dom over the preloaded plain-object document — Redirect renders
+// through a real TanStack router, which needs a DOM. Bun runs test files
+// sequentially in one process and happy-dom's globals are process-wide, so
+// we MUST unregister in afterAll (below) to restore the shared mock document
+// for the other renderer suites — otherwise readonly DOM globals leak and
+// break unrelated tests that assign e.g. globalThis.localStorage.
+const alreadyRegistered = GlobalRegistrator.isRegistered;
+if (!alreadyRegistered) GlobalRegistrator.register();
+(
+	globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }
+).IS_REACT_ACT_ENVIRONMENT = true;
+
+const { act, cleanup, render } = await import("@testing-library/react");
+const {
+	createMemoryHistory,
+	createRootRoute,
+	createRoute,
+	createRouter,
+	Outlet,
+	RouterProvider,
+} = await import("@tanstack/react-router");
+const React = await import("react");
+const { Redirect: TypedRedirect } = await import("./Redirect");
+
+// The real Redirect types `to` against the app's registered route tree; this
+// test drives a synthetic router with routes that don't exist there, so widen
+// the signature. Runtime behavior is unchanged — this only relaxes the paths.
+const Redirect = TypedRedirect as (props: {
+	to: string;
+	replace?: boolean;
+	search?: Record<string, unknown>;
+}) => React.ReactElement | null;
+
+afterEach(cleanup);
+afterAll(async () => {
+	if (!alreadyRegistered) await GlobalRegistrator.unregister();
+});
+
+/**
+ * Build a router that renders `children` in the ALWAYS-MOUNTED root component
+ * (so a redirect element survives the navigation it triggers), plus inert
+ * destination routes. `navigations` counts every router.navigate call so a
+ * re-render loop shows up as a rising count.
+ */
+function setup(children: React.ReactNode) {
+	const rootRoute = createRootRoute({
+		component: () => (
+			<>
+				{children}
+				<Outlet />
+			</>
+		),
+	});
+	const indexRoute = createRoute({
+		getParentRoute: () => rootRoute,
+		path: "/",
+		component: () => <div>index</div>,
+	});
+	const targetRoute = createRoute({
+		getParentRoute: () => rootRoute,
+		path: "/target",
+		component: () => <div>target</div>,
+	});
+	const otherRoute = createRoute({
+		getParentRoute: () => rootRoute,
+		path: "/other",
+		component: () => <div>other</div>,
+	});
+	const router = createRouter({
+		routeTree: rootRoute.addChildren([indexRoute, targetRoute, otherRoute]),
+		history: createMemoryHistory({ initialEntries: ["/"] }),
+	});
+
+	let navigations = 0;
+	const realNavigate = router.navigate.bind(router);
+	router.navigate = ((opts: Parameters<typeof realNavigate>[0]) => {
+		navigations += 1;
+		return realNavigate(opts);
+	}) as typeof router.navigate;
+
+	return { router, getNavigations: () => navigations };
+}
+
+describe("Redirect", () => {
+	test("navigates to the target once", async () => {
+		const { router } = setup(<Redirect to="/target" replace />);
+		await act(async () => {
+			render(<RouterProvider router={router} />);
+			await router.latestLoadPromise;
+		});
+		expect(router.state.location.pathname).toBe("/target");
+	});
+
+	test("does not re-navigate when re-rendered with fresh props identity", async () => {
+		// The exact condition that loops TanStack's <Navigate>: a parent that
+		// re-renders and hands the redirect a new props object every time.
+		let forceRender: (() => void) | undefined;
+		function Parent() {
+			const [, setTick] = React.useState(0);
+			forceRender = () => setTick((n) => n + 1);
+			// New object literal each render → new props identity for Redirect.
+			return <Redirect to={"/target"} replace search={{}} />;
+		}
+
+		const { router, getNavigations } = setup(<Parent />);
+		await act(async () => {
+			render(<RouterProvider router={router} />);
+			await router.latestLoadPromise;
+		});
+
+		const afterMount = getNavigations();
+		expect(router.state.location.pathname).toBe("/target");
+
+		for (let i = 0; i < 10; i++) {
+			await act(async () => {
+				forceRender?.();
+				await router.latestLoadPromise;
+			});
+		}
+
+		// The whole point: identity churn must not add navigations.
+		expect(getNavigations()).toBe(afterMount);
+		expect(router.state.location.pathname).toBe("/target");
+	});
+
+	test("re-navigates when the resolved target actually changes", async () => {
+		let setTarget: ((to: "/target" | "/other") => void) | undefined;
+		function Parent() {
+			const [to, set] = React.useState<"/target" | "/other">("/target");
+			setTarget = set;
+			return <Redirect to={to} replace />;
+		}
+
+		const { router, getNavigations } = setup(<Parent />);
+		await act(async () => {
+			render(<RouterProvider router={router} />);
+			await router.latestLoadPromise;
+		});
+		expect(router.state.location.pathname).toBe("/target");
+		const afterFirst = getNavigations();
+
+		await act(async () => {
+			setTarget?.("/other");
+			await router.latestLoadPromise;
+		});
+
+		expect(getNavigations()).toBeGreaterThan(afterFirst);
+		expect(router.state.location.pathname).toBe("/other");
+	});
+
+	test("renders nothing", async () => {
+		const { router } = setup(<Redirect to="/target" replace />);
+		let html = "";
+		await act(async () => {
+			const { container } = render(<RouterProvider router={router} />);
+			await router.latestLoadPromise;
+			// target route renders <div>target</div>; Redirect itself emits nothing
+			html = container.innerHTML;
+		});
+		expect(html).not.toContain("undefined");
+	});
+});

apps/desktop/src/renderer/components/Redirect/Redirect.tsx

@@ -0,0 +1,38 @@
+import {
+	type NavigateOptions,
+	useNavigate,
+	useRouter,
+} from "@tanstack/react-router";
+import { useEffect, useRef } from "react";
+
+/**
+ * Loop-proof declarative redirect. TanStack's <Navigate> re-navigates
+ * whenever its props OBJECT IDENTITY changes, so an inline element
+ * re-navigates on every parent re-render until React throws error #185
+ * (#5729, SUPER-1814). This keys off the resolved target href instead:
+ * one navigation per distinct destination, safe to render inline with
+ * dynamic params/search. Prefer `beforeLoad: () => { throw redirect(...) }`
+ * when the target is static — that avoids render-time navigation entirely.
+ */
+export function Redirect(props: NavigateOptions) {
+	const router = useRouter();
+	const navigate = useNavigate();
+	const href = router.buildLocation(props).href;
+	const propsRef = useRef(props);
+	propsRef.current = props;
+	const lastHrefRef = useRef<string | null>(null);
+
+	useEffect(() => {
+		if (lastHrefRef.current === href) return;
+		navigate(propsRef.current).catch((error) => {
+			// A route-load/guard failure would otherwise be an unhandled
+			// rejection. Clear the marker so a later render can retry the
+			// same target rather than treating it as permanently done.
+			lastHrefRef.current = null;
+			console.error("[Redirect] navigation failed", { href, error });
+		});
+		lastHrefRef.current = href;
+	}, [href, navigate]);
+
+	return null;
+}

apps/desktop/src/renderer/components/Redirect/index.ts

@@ -0,0 +1 @@
+export { Redirect } from "./Redirect";

apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardContentError/DashboardContentError.tsx

@@ -0,0 +1,54 @@
+import { Button } from "@superset/ui/button";
+import type { ErrorComponentProps } from "@tanstack/react-router";
+import { Link } from "@tanstack/react-router";
+import { useEffect } from "react";
+import { HiExclamationTriangle } from "react-icons/hi2";
+
+/**
+ * Content-pane error state for the dashboard. Rendered by the CatchBoundary
+ * around the dashboard Outlet so a crashing route unmounts only the content
+ * area — the sidebar, terminals, and window chrome stay alive (SUPER-1814
+ * presented as "the whole app restarts" because errors used to bubble to the
+ * root boundary). The boundary resets on navigation, so the sidebar and the
+ * Go-home link both recover it.
+ */
+export function DashboardContentError({ error }: ErrorComponentProps) {
+	const message =
+		error instanceof Error ? error.message : String(error ?? "Unknown error");
+
+	useEffect(() => {
+		console.error("[dashboard] Content route error caught:", error);
+		void import("@sentry/electron/renderer")
+			.then((Sentry) => Sentry.captureException(error))
+			.catch((reportError) => {
+				// Don't let a telemetry failure vanish silently — the fallback UI
+				// still renders, but we want the reporting gap to be observable.
+				console.error(
+					"[dashboard] failed to report content error to Sentry",
+					reportError,
+				);
+			});
+	}, [error]);
+
+	return (
+		<div className="flex h-full w-full flex-col items-center justify-center gap-4 bg-background px-8">
+			<div className="flex h-12 w-12 items-center justify-center rounded-full bg-destructive/10">
+				<HiExclamationTriangle className="h-6 w-6 text-destructive" />
+			</div>
+			<div className="flex flex-col items-center gap-1 text-center">
+				<h2 className="text-base font-semibold">This view hit an error</h2>
+				<p className="max-w-md text-sm text-muted-foreground select-text cursor-text break-words">
+					{message}
+				</p>
+			</div>
+			<div className="flex items-center gap-2">
+				<Button variant="outline" size="sm" asChild>
+					<Link to="/">Go home</Link>
+				</Button>
+				<Button size="sm" onClick={() => window.location.reload()}>
+					Reload app
+				</Button>
+			</div>
+		</div>
+	);
+}

apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardContentError/index.ts

@@ -0,0 +1 @@
+export { DashboardContentError } from "./DashboardContentError";

apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx

@@ -1,12 +1,14 @@
 import {
+	CatchBoundary,
 	createFileRoute,
-	Navigate,
 	Outlet,
+	useLocation,
 	useMatchRoute,
 	useNavigate,
 } from "@tanstack/react-router";
 import { useMemo, useState } from "react";
 import { CommandPaletteHost } from "renderer/commandPalette";
+import { Redirect } from "renderer/components/Redirect";
 import { useIsV2CloudEnabled } from "renderer/hooks/useIsV2CloudEnabled";
 import { useHotkey } from "renderer/hotkeys";
 import { electronTrpc } from "renderer/lib/electron-trpc";
@@ -26,17 +28,13 @@ import {
 } from "renderer/stores/workspace-sidebar-state";
 import { AddRepositoryModals } from "./components/AddRepositoryModals";
 import { CrossVersionMismatchState } from "./components/CrossVersionMismatchState";
+import { DashboardContentError } from "./components/DashboardContentError";
 import { TopBar } from "./components/TopBar";
 
 export const Route = createFileRoute("/_authenticated/_dashboard")({
 	component: DashboardLayout,
 });
 
-// Hoisted for stable props identity — <Navigate> re-navigates whenever its
-// props object changes, so an inline element re-navigates on every re-render
-// until React throws error #185 (see routes/page.tsx, #5729, SUPER-1814).
-const newWorkspaceRedirect = <Navigate to="/new-workspace" replace />;
-
 /** v1 only — v2 deletes go through the globally-mounted DeleteWorkspaceMount
  * (see delete-workspace-intent store). */
 type DeleteTarget = {
@@ -47,6 +45,7 @@ type DeleteTarget = {
 
 function DashboardLayout() {
 	const navigate = useNavigate();
+	const location = useLocation();
 	const openNewWorkspaceModal = useOpenNewWorkspaceModal();
 	const isV2CloudEnabled = useIsV2CloudEnabled();
 	const { workspaces: hostWorkspaces } = useHostWorkspaces();
@@ -230,12 +229,23 @@ function DashboardLayout() {
 							// dead-end "pick a workspace" screen. v1 users keep the
 							// static state — /new-workspace is a v2-only surface.
 							isV2CloudEnabled ? (
-								newWorkspaceRedirect
+								<Redirect to="/new-workspace" replace />
 							) : (
 								<CrossVersionMismatchState />
 							)
 						) : (
-							<Outlet />
+							// Contain content-route crashes to this pane: without a
+							// boundary they bubble to the root and unmount the whole
+							// app, which reads as Superset restarting itself
+							// (SUPER-1814). Resets on navigation.
+							<CatchBoundary
+								// Full href, not just pathname: a same-path search/hash
+								// change (filter, tab) must also clear a stuck error pane.
+								getResetKey={() => location.href}
+								errorComponent={DashboardContentError}
+							>
+								<Outlet />
+							</CatchBoundary>
 						)}
 					</div>
 				</div>