diff --git a/.changeset/hungry-donkeys-repeat.md b/.changeset/hungry-donkeys-repeat.md new file mode 100644 index 0000000000..dcdeaeac24 --- /dev/null +++ b/.changeset/hungry-donkeys-repeat.md @@ -0,0 +1,20 @@ +--- +"@agent-native/core": patch +--- + +Stop stranding users on the loading spinner when the session endpoint is +unreadable. `useSession` retried a failed `/_agent-native/auth/session` every +second forever while holding `isLoading` true, so a transient 5xx, network +failure, or timeout produced a spinner that never resolved and carried no error +anywhere. It now retries a bounded number of times with backoff and then reports +a distinct `status: "unavailable"` alongside the existing `session`/`isLoading` +fields. + +`RequireSession` keys off that status: unreadable is no longer collapsed into +signed-out (which would bounce a signed-in user to the sign-in page over a blip) +nor into loading (which stranded them). It renders a notice with Try again and +Reload actions instead. + +The `DefaultSpinner` stall hint is also environment-aware now. It previously +told every visitor — including on hosted deployments — to "check the terminal +running the dev server", which is meaningless outside local development. diff --git a/packages/core/src/client/app-providers.spec.tsx b/packages/core/src/client/app-providers.spec.tsx index 55f7484fea..8f62abfea1 100644 --- a/packages/core/src/client/app-providers.spec.tsx +++ b/packages/core/src/client/app-providers.spec.tsx @@ -74,9 +74,18 @@ function renderProviders(props: { }); } +// `RequireSession` branches on `useSession().status`, not just `isLoading` — +// every mock here must supply a status or the gate can neither redirect nor +// hold the fallback consistently with the real hook. +const SIGNED_OUT_SESSION = { + session: null, + isLoading: false, + status: "unauthenticated" as const, +}; + describe("AppProviders session gate", () => { it("uses Toolkit's theme-aware toaster by default", () => { - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue(SIGNED_OUT_SESSION); act(() => { root.render( @@ -92,7 +101,7 @@ describe("AppProviders session gate", () => { }); it("renders public paths directly without resolving or redirecting a session", () => { - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue(SIGNED_OUT_SESSION); renderProviders({ isPublicPath: true }); @@ -104,7 +113,7 @@ describe("AppProviders session gate", () => { }); it("gates private paths and redirects signed-out visitors after hydration", () => { - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue(SIGNED_OUT_SESSION); renderProviders({}); @@ -116,7 +125,7 @@ describe("AppProviders session gate", () => { }); it("allows token-authenticated private surfaces to bypass the session gate", () => { - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue(SIGNED_OUT_SESSION); renderProviders({ sessionBypass: true }); diff --git a/packages/core/src/client/require-session.spec.tsx b/packages/core/src/client/require-session.spec.tsx index aca313dcad..1e8504dd0c 100644 --- a/packages/core/src/client/require-session.spec.tsx +++ b/packages/core/src/client/require-session.spec.tsx @@ -82,7 +82,11 @@ const Child = () =>
inbox
; describe("RequireSession", () => { it("shows a loading fallback while the session resolves and never redirects", () => { - useSessionMock.mockReturnValue({ session: null, isLoading: true }); + useSessionMock.mockReturnValue({ + session: null, + isLoading: true, + status: "loading", + }); render( @@ -97,6 +101,7 @@ describe("RequireSession", () => { useSessionMock.mockReturnValue({ session: { userId: "u1", email: "a@b.com" }, isLoading: false, + status: "authenticated", }); render( @@ -108,7 +113,11 @@ describe("RequireSession", () => { }); it("redirects to the framework sign-in page carrying an opaque continuation", () => { - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue({ + session: null, + isLoading: false, + status: "unauthenticated", + }); render( @@ -131,7 +140,11 @@ describe("RequireSession", () => { // here, which is the only thing left standing between this surface and a // same-URL replace loop — it must never gain a fallback. stubLocation("/_agent-native/sign-in", "?c=abc"); - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue({ + session: null, + isLoading: false, + status: "unauthenticated", + }); render( @@ -146,7 +159,11 @@ describe("RequireSession", () => { // old marker-only base resolver returned "" and failed to recognise it as // an auth entry path — a live, reproducible infinite bounce. vi.stubEnv("VITE_APP_BASE_PATH", "/myapp"); - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue({ + session: null, + isLoading: false, + status: "unauthenticated", + }); for (const path of ["/myapp/login", "/myapp/signup"]) { stubLocation(path); render( @@ -160,7 +177,11 @@ describe("RequireSession", () => { }); it("does not redirect twice across re-renders", () => { - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue({ + session: null, + isLoading: false, + status: "unauthenticated", + }); render( @@ -175,7 +196,11 @@ describe("RequireSession", () => { }); it("renders `signedOut` instead of redirecting when redirect is disabled", () => { - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue({ + session: null, + isLoading: false, + status: "unauthenticated", + }); render( please sign in}> @@ -185,8 +210,35 @@ describe("RequireSession", () => { expect(replaceMock).not.toHaveBeenCalled(); }); + it("shows a recoverable notice when the session is unreadable", () => { + // A transient 5xx must read as neither "signed out" (which bounces a + // signed-in user to sign-in) nor "still loading" (which strands them). + useSessionMock.mockReturnValue({ + session: null, + // The real hook keeps isLoading true for "unavailable" so legacy + // isLoading-only consumers never misread it as signed-out. + isLoading: true, + status: "unavailable", + error: new Error("Could not read the session after 4 attempts."), + retry: vi.fn(), + }); + render( + + + , + ); + expect(replaceMock).not.toHaveBeenCalled(); + expect(container.querySelector('[data-testid="protected"]')).toBeNull(); + expect(container.querySelector('[aria-label="Loading"]')).toBeNull(); + expect(container.textContent).toContain("Try again"); + }); + it("bypass renders children even with no session", () => { - useSessionMock.mockReturnValue({ session: null, isLoading: false }); + useSessionMock.mockReturnValue({ + session: null, + isLoading: false, + status: "unauthenticated", + }); render( diff --git a/packages/core/src/client/require-session.tsx b/packages/core/src/client/require-session.tsx index 419b5bd023..d464488e16 100644 --- a/packages/core/src/client/require-session.tsx +++ b/packages/core/src/client/require-session.tsx @@ -118,13 +118,13 @@ function ResolvedSessionGate({ redirect = true, signedOut, }: Omit) { - const { session, isLoading } = useSession(); + const { session, status, retry } = useSession(); // Guard against firing the redirect more than once (effect re-runs, React // StrictMode double-invoke) — a second navigation while the first is in // flight is harmless but noisy. const redirectedRef = useRef(false); - const mustRedirect = !isLoading && !session && redirect; + const mustRedirect = status === "unauthenticated" && redirect; useEffect(() => { if (!mustRedirect) return; @@ -144,10 +144,43 @@ function ResolvedSessionGate({ // Still resolving, or redirect already in flight: show the loading fallback // rather than flashing app chrome the visitor can't use. - if (isLoading) return <>{fallback ?? }; + if (status === "loading") return <>{fallback ?? }; + // Unreadable is not signed-out. Redirecting here would bounce a signed-in + // user to the sign-in page over a transient 5xx, and rendering the spinner + // would strand them on a screen that never resolves. + if (status === "unavailable") { + return ; + } if (!session) { if (redirect) return <>{fallback ?? }; return <>{signedOut ?? null}; } return <>{children}; } + +function SessionUnavailableNotice({ retry }: { retry: () => void }) { + return ( +
+

+ We couldn't reach the server to confirm you're signed in. This + is usually temporary. +

+
+ + +
+
+ ); +} diff --git a/packages/core/src/client/use-session.spec.tsx b/packages/core/src/client/use-session.spec.tsx index bff21465f6..04308385dc 100644 --- a/packages/core/src/client/use-session.spec.tsx +++ b/packages/core/src/client/use-session.spec.tsx @@ -29,6 +29,11 @@ function SessionConsumers({ labels }: { labels: string[] }) { return labels.map((label) => ); } +function StatusConsumer() { + const { status } = useSession(); + return
{status}
; +} + async function renderConsumers(labels: string[]) { await act(async () => { root.render(); @@ -144,6 +149,47 @@ describe("useSession", () => { expect(analyticsMocks.trackSessionStatus).toHaveBeenCalledWith(true); }); + it("stops retrying and reports unavailable when the endpoint keeps failing", async () => { + vi.useFakeTimers(); + const failingFetch = vi.fn(async () => new Response(null, { status: 503 })); + vi.stubGlobal("fetch", failingFetch); + + await act(async () => { + root.render(); + await Promise.resolve(); + }); + expect(container.textContent).toBe("loading"); + + await act(async () => { + await vi.advanceTimersByTimeAsync(30_000); + }); + + expect(container.textContent).toBe("unavailable"); + expect(failingFetch).toHaveBeenCalledTimes(4); + // An unreadable endpoint must never be reported as a signed-out visitor. + expect(analyticsMocks.trackSessionStatus).not.toHaveBeenCalled(); + }); + + it("keeps legacy isLoading consumers from misreading unavailable as signed-out", async () => { + // Consumers that only read `isLoading`/`session` (not `status`) must never + // see a false "signed out" once retries are exhausted. + vi.useFakeTimers(); + const failingFetch = vi.fn(async () => new Response(null, { status: 503 })); + vi.stubGlobal("fetch", failingFetch); + + await act(async () => { + root.render(); + await Promise.resolve(); + }); + + await act(async () => { + await vi.advanceTimersByTimeAsync(30_000); + }); + + expect(failingFetch).toHaveBeenCalledTimes(4); + expect(container.textContent).toBe("loading"); + }); + it("caches a definitive unauthenticated response", async () => { const fetchMock = vi.fn(async () => jsonResponse({ error: "Not authenticated" }), diff --git a/packages/core/src/client/use-session.ts b/packages/core/src/client/use-session.ts index c24b18aacb..86b23c6b83 100644 --- a/packages/core/src/client/use-session.ts +++ b/packages/core/src/client/use-session.ts @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import { useCallback, useEffect, useState } from "react"; import type { AuthSession } from "../server/auth.js"; import { setSentryUser, trackSessionStatus } from "./analytics.js"; @@ -6,13 +6,30 @@ import { fetchAuthSessionStatus } from "./client-status-requests.js"; export type { AuthSession }; +/** + * `"unavailable"` is the session endpoint being unreadable — a 5xx, a network + * failure, or a timeout. It is NOT the visitor being signed out, and a caller + * that collapses the two either strands the user on a spinner forever or + * bounces a signed-in user to the sign-in page over a transient blip. + */ +export type SessionStatus = + | "loading" + | "authenticated" + | "unauthenticated" + | "unavailable"; + interface UseSessionResult { session: AuthSession | null; isLoading: boolean; + status: SessionStatus; + error: Error | null; + /** Restart the resolve loop, e.g. from a "Try again" control. */ + retry: () => void; } const SESSION_CACHE_TTL_MS = 30_000; const SESSION_RETRY_DELAY_MS = 1_000; +const SESSION_MAX_ATTEMPTS = 4; let cachedSession: AuthSession | null | undefined; let cachedSessionAt = 0; let sessionRequest: Promise | undefined; @@ -81,25 +98,46 @@ function fetchSharedSession(): Promise { export function useSession(): UseSessionResult { const cached = hasFreshSessionCache() ? (cachedSession ?? null) : null; const [session, setSession] = useState(cached); - const [isLoading, setIsLoading] = useState(!hasFreshSessionCache()); + const [status, setStatus] = useState(() => { + if (!hasFreshSessionCache()) return "loading"; + return cached ? "authenticated" : "unauthenticated"; + }); + const [error, setError] = useState(null); + const [retryToken, setRetryToken] = useState(0); + + const retry = useCallback(() => { + setError(null); + setStatus("loading"); + setRetryToken((token) => token + 1); + }, []); useEffect(() => { let cancelled = false; let retryTimer: ReturnType | undefined; + let attempts = 0; const resolveSession = async () => { const resolved = await fetchSharedSession(); if (cancelled) return; if (resolved === undefined) { + attempts += 1; + if (attempts >= SESSION_MAX_ATTEMPTS) { + setError( + new Error(`Could not read the session after ${attempts} attempts.`), + ); + setStatus("unavailable"); + return; + } retryTimer = setTimeout(() => { void resolveSession(); - }, SESSION_RETRY_DELAY_MS); + }, SESSION_RETRY_DELAY_MS * attempts); return; } setSession(resolved); - setIsLoading(false); + setError(null); + setStatus(resolved ? "authenticated" : "unauthenticated"); }; void resolveSession(); @@ -107,7 +145,15 @@ export function useSession(): UseSessionResult { cancelled = true; if (retryTimer) clearTimeout(retryTimer); }; - }, []); - - return { session, isLoading }; + }, [retryToken]); + + // Callers that only read `isLoading`/`session` (most of the codebase, not + // yet migrated to `status`) must not see "unavailable" as "signed out" — + // that bounces an authenticated user through sign-in-only UI over a + // transient blip. Keeping `isLoading` true here reproduces this hook's + // pre-existing behavior for those callers (an indefinite "still resolving" + // instead of a wrong answer); only `status`-aware callers get the distinct + // "unavailable" treatment with a retry affordance. + const isLoading = status === "loading" || status === "unavailable"; + return { session, isLoading, status, error, retry }; }