Fix background fetcher loads folded into navigations

app/features/layout/LayoutDataProvider.tsx

@@ -1,7 +1,8 @@
 import * as React from "react";
-import { useFetcher } from "react-router";
+import { useBackgroundResource } from "~/hooks/useBackgroundResource";
 import { useReloadOnNewDeploy } from "~/hooks/useReloadOnNewDeploy";
 import type { RootLoaderData } from "~/root";
+import type { SerializeFrom } from "~/utils/remix";
 import { LAYOUT_DATA_ROUTE } from "~/utils/urls";
 import type { loader } from "./routes/api.layout";
 
@@ -38,18 +39,21 @@ export function LayoutDataProvider({
 	data?: RootLoaderData;
 	children: React.ReactNode;
 }) {
-	const fetcher = useFetcher<typeof loader>();
-	const { load, state } = fetcher;
+	const {
+		data: polledData,
+		isLoading,
+		refresh,
+	} = useBackgroundResource<SerializeFrom<typeof loader>>(LAYOUT_DATA_ROUTE);
 
 	// read through a ref so a poll elsewhere in the app does not re-run the effect
 	// and restart the interval before it ever fires
-	const stateRef = React.useRef(state);
-	stateRef.current = state;
+	const isLoadingRef = React.useRef(isLoading);
+	isLoadingRef.current = isLoading;
 
 	React.useEffect(() => {
 		const loadIfIdle = () => {
-			if (stateRef.current === "idle") {
-				load(LAYOUT_DATA_ROUTE);
+			if (!isLoadingRef.current) {
+				void refresh();
 			}
 		};
 
@@ -66,12 +70,9 @@ export function LayoutDataProvider({
 			document.removeEventListener("visibilitychange", handleVisibilityChange);
 			clearInterval(interval);
 		};
-	}, [load]);
+	}, [refresh]);
 
-	// stable so effects that refresh after a mutation don't re-run every render
-	const refresh = React.useCallback(() => load(LAYOUT_DATA_ROUTE), [load]);
-
-	const newest = useNewestOf(data, fetcher.data);
+	const newest = useNewestOf(data, polledData);
 
 	useReloadOnNewDeploy(newest.buildCommit ?? "");
 	useReloadOnStaleAuth({
@@ -82,7 +83,7 @@ export function LayoutDataProvider({
 	const value: LayoutDataContextValue = {
 		...newest,
 		refresh,
-		isRefreshing: state !== "idle",
+		isRefreshing: isLoading,
 	};
 
 	return (

app/features/notifications/NotificationsProvider.tsx

@@ -1,11 +1,7 @@
 import * as React from "react";
-import {
-	useFetcher,
-	useFetchers,
-	useLocation,
-	useNavigation,
-} from "react-router";
+import { useFetchers, useLocation, useNavigation } from "react-router";
 import { useChatContext } from "~/features/chat/useChatContext";
+import { useBackgroundResource } from "~/hooks/useBackgroundResource";
 import type { SerializeFrom } from "~/utils/remix";
 import { NOTIFICATIONS_DATA_ROUTE } from "~/utils/urls";
 import { resyncPushSubscription } from "./core/pushSubscription";
@@ -44,20 +40,15 @@ export function NotificationsProvider({
 	user?: { id: number } | null;
 	children: React.ReactNode;
 }) {
-	const fetcher = useFetcher<typeof loader>();
+	const { data, refresh } = useBackgroundResource<SerializeFrom<typeof loader>>(
+		NOTIFICATIONS_DATA_ROUTE,
+	);
 	const chat = useChatContext();
-	const { load } = fetcher;
 
 	const loggedIn = Boolean(user);
 	const readyState = chat?.readyState ?? "CLOSED";
 	const wsDown = loggedIn && readyState !== "CONNECTED";
 
-	// stable so effects that refresh after a mutation don't re-run every render
-	const refresh = React.useCallback(
-		() => load(NOTIFICATIONS_DATA_ROUTE),
-		[load],
-	);
-
 	React.useEffect(() => {
 		if (!loggedIn) return;
 
@@ -70,7 +61,7 @@ export function NotificationsProvider({
 	useRefreshOnVisible({ enabled: loggedIn, refresh });
 	useFallbackPoll({ enabled: wsDown, refresh });
 
-	const notifications = fetcher.data?.notifications;
+	const notifications = data?.notifications;
 
 	useFallbackRefreshOnPotentialResolution({
 		enabled: wsDown,

app/hooks/useBackgroundResource.ts

@@ -0,0 +1,38 @@
+import * as React from "react";
+
+/**
+ * Keeps an app shell resource route's JSON fresh, outside the router. A
+ * `useFetcher` load that is still in flight when a navigation starts is folded
+ * into that navigation and has to settle before it completes (React Router
+ * reruns cancelled fetcher loads without consulting `shouldRevalidate`), so a
+ * refresh that only feeds the app shell would hold up every page change it
+ * happens to overlap with.
+ */
+export function useBackgroundResource<T>(url: string) {
+	const [data, setData] = React.useState<T>();
+	const [isLoading, setIsLoading] = React.useState(false);
+	const latestRequestRef = React.useRef(0);
+
+	// stable so effects that refresh after a mutation don't re-run every render
+	const refresh = React.useCallback(async () => {
+		const requestId = ++latestRequestRef.current;
+		setIsLoading(true);
+
+		try {
+			const response = await fetch(url);
+			if (!response.ok) return;
+
+			const json = (await response.json()) as T;
+			// a newer refresh already started, its response is the fresher one
+			if (requestId !== latestRequestRef.current) return;
+
+			setData(json);
+		} catch {
+			// a background refresh failing just leaves the last data in place
+		} finally {
+			if (requestId === latestRequestRef.current) setIsLoading(false);
+		}
+	}, [url]);
+
+	return { data, isLoading, refresh };
+}

app/root.tsx

@@ -432,16 +432,27 @@ function HydrationTestIndicator() {
 
 	if (!isHydrated) return null;
 
-	const routerIdle =
-		navigation.state === "idle" &&
-		revalidator.state === "idle" &&
-		fetchers.every((fetcher) => fetcher.state === "idle");
+	const busy = [
+		navigation.state !== "idle"
+			? `nav:${navigation.state}:${navigation.location?.pathname}`
+			: null,
+		revalidator.state !== "idle" ? `revalidator:${revalidator.state}` : null,
+		...fetchers
+			.filter((fetcher) => fetcher.state !== "idle")
+			.map(
+				(fetcher) =>
+					`fetcher[${fetcher.key}]:${fetcher.state}:${fetcher.formAction ?? "load"}`,
+			),
+	].filter(Boolean);
+
+	const routerIdle = busy.length === 0;
 
 	return (
 		<div
 			style={{ display: "none" }}
 			data-testid="hydrated"
 			data-router-idle={routerIdle ? "true" : undefined}
+			data-router-busy={routerIdle ? undefined : busy.join(" | ")}
 		/>
 	);
 }

e2e/helpers/playwright.ts

@@ -318,11 +318,25 @@ export async function waitForPOSTResponse(page: Page, cb: () => Promise<void>) {
 async function expectRouterIdle(page: Page) {
 	// A submit's redirect plus the target page's loaders can exceed the default
 	// expect timeout when the full suite is loading all workers.
-	await expect(page.getByTestId("hydrated")).toHaveAttribute(
-		"data-router-idle",
-		"true",
-		{ timeout: 15_000 },
-	);
+	try {
+		await expect(page.getByTestId("hydrated")).toHaveAttribute(
+			"data-router-idle",
+			"true",
+			{ timeout: 15_000 },
+		);
+	} catch (error) {
+		// data-router-busy names what is still in flight, which the attribute
+		// assertion's own message does not
+		const busy = await page
+			.getByTestId("hydrated")
+			.getAttribute("data-router-busy")
+			.catch(() => null);
+
+		throw new Error(
+			`Router never went idle at ${page.url()} (in flight: ${busy ?? "unknown"})`,
+			{ cause: error },
+		);
+	}
 }
 
 /** Asserts the page rendered rather than the error boundary catching something. */