diff --git a/app/features/layout/LayoutDataProvider.tsx b/app/features/layout/LayoutDataProvider.tsx index 43a640362..f3f217c3f 100644 --- a/app/features/layout/LayoutDataProvider.tsx +++ b/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(); - const { load, state } = fetcher; + const { + data: polledData, + isLoading, + refresh, + } = useBackgroundResource>(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 ( diff --git a/app/features/notifications/NotificationsProvider.tsx b/app/features/notifications/NotificationsProvider.tsx index 2583e74aa..586c4bfa2 100644 --- a/app/features/notifications/NotificationsProvider.tsx +++ b/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(); + const { data, refresh } = useBackgroundResource>( + 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, diff --git a/app/hooks/useBackgroundResource.ts b/app/hooks/useBackgroundResource.ts new file mode 100644 index 000000000..4851ff769 --- /dev/null +++ b/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(url: string) { + const [data, setData] = React.useState(); + 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 }; +} diff --git a/app/root.tsx b/app/root.tsx index feae410e7..b875f5c62 100644 --- a/app/root.tsx +++ b/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 (
); } diff --git a/e2e/helpers/playwright.ts b/e2e/helpers/playwright.ts index 5bc2c05a2..bd7638ab3 100644 --- a/e2e/helpers/playwright.ts +++ b/e2e/helpers/playwright.ts @@ -318,11 +318,25 @@ export async function waitForPOSTResponse(page: Page, cb: () => Promise) { 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. */