fix(auth): recover from a failed OAuth callback instead of wedging

.changeset/olive-pugs-repeat.md

@@ -0,0 +1,15 @@
+---
+"@tailor-platform/app-shell": patch
+---
+
+Fix `autoLogin` being permanently disabled after a failed OAuth callback.
+
+`@tailor-platform/auth-public-client` cleans the callback parameters out of the URL only when the code exchange succeeds; every failure path leaves `?code=` or `?error=` in the query string (tailor-platform/auth-public-client#139). `AuthProvider` decided whether a callback was in progress by reading that URL, so after any failed callback it treated the page as a live callback forever: auto-login never fired again, the app sat on `guardComponent`, and reloading only replayed the same failing callback.
+
+Auto-login now decides from the callback's status rather than the URL — which is what the check meant all along: "is an exchange in flight", not "has this page ever been a callback". The URL is still the authority when no callback has been claimed at all, where redirecting away from unconsumed parameters would be unsafe.
+
+It will not loop, either. A recoverable failure is retried once per tab; beyond that, and for any refusal the authorization server issues explicitly, AppShell stops rather than redirecting the user back for the same answer. The outcome is classified from the resulting auth state rather than from whether the callback threw, because two failure paths — a server `error` and a state mismatch — return normally after recording the error.
+
+That makes `guardComponent` where a failed sign-in becomes visible: a guard that only renders a spinner will spin indefinitely, so render `useAuth().error` and offer a retry. See the authentication guide.
+
+The stale parameters themselves remain in the URL after a failure until the cleanup is fixed upstream (tailor-platform/auth-public-client#139) — app-shell no longer misbehaves because of them, but deliberately does not reimplement the library's URL cleanup.

docs/concepts/authentication.md

@@ -217,6 +217,31 @@ function ChatScreen() {
 | `autoLogin`      | `boolean`               | No       | Automatically redirect unauthenticated users to login |
 | `guardComponent` | `() => React.ReactNode` | No       | Rendered while loading or when not authenticated      |
 
+## When the callback fails
+
+A sign-in that comes back from the authorization server unsuccessfully — the user declined, the client is not permitted, the exchange failed — leaves `isAuthenticated` false with the reason on `useAuth().error`. The stale callback parameters may remain in the URL (the auth client currently cleans them only on success — tracked upstream as `tailor-platform/auth-public-client#139`; the repo is private, hence no link), but they no longer affect AppShell's behaviour.
+
+With `autoLogin`, a failure that looks recoverable is retried once automatically. Beyond that, and for any refusal the authorization server issues explicitly, AppShell stops: sending the user straight back would ask the same question, get the same answer, and loop.
+
+That makes the guard the place where a failed sign-in becomes visible. A guard that only ever renders a spinner will spin indefinitely in this case, so render the error and offer a way out:
+
+```tsx
+const AuthGate = () => {
+  const { isReady, error, login } = useAuth();
+
+  if (error) {
+    return (
+      <div>
+        <p>Sign-in failed: {error}</p>
+        <button onClick={() => login()}>Try again</button>
+      </div>
+    );
+  }
+
+  return isReady ? <LoginPrompt /> : <LoadingScreen />;
+};
+```
+
 ## Integration with AppShell
 
 The authentication provider works seamlessly with AppShell's data layer, automatically handling:

packages/core/src/contexts/auth-context.test.tsx

@@ -23,6 +23,9 @@ afterEach(() => {
   vi.clearAllMocks();
   vi.unstubAllGlobals();
   window.history.replaceState({}, "", "/");
+  // The failed-callback retry counter is tab-scoped and survives page loads by
+  // design, so it has to be reset between tests or it leaks across them.
+  window.sessionStorage.clear();
 });
 
 const LoadingGuard = () => <div>Loading...</div>;
@@ -955,6 +958,205 @@ describe("createAuthClient", () => {
     expect(mockHandleCallback).toHaveBeenCalledTimes(1);
   });
 
+  describe("failed callback recovery", () => {
+    const renderWithClient = (
+      baseOverrides: Record<string, unknown>,
+      handleCallback: ReturnType<typeof vi.fn>,
+    ) => {
+      vi.mocked(createAuthClientMock).mockReturnValue({
+        ...makeBaseClient(handleCallback),
+        ...baseOverrides,
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      } as any);
+
+      return createAuthClient({ clientId: "test", appUri: "https://test.com" });
+    };
+
+    // A client whose callback has already settled, so the gate must consult the
+    // status rather than the (still dirty) URL.
+    const createSettledCallbackClient = (
+      status: "resolved" | "rejected",
+      overrides: Record<string, unknown>,
+    ) =>
+      ({
+        ...makeBaseClient(vi.fn()),
+        getAppUri: vi.fn(() => "https://api.test.com"),
+        getCallbackStatusSnapshot: () => status,
+        subscribeCallbackStatus: () => () => {},
+        ...overrides,
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      }) as any;
+
+    const readyUnauthenticated = {
+      isAuthenticated: false,
+      error: null as string | null,
+      isReady: true,
+    };
+
+    it("resumes auto-login after a failed code exchange", async () => {
+      // The dead end this guards: before the parameters were cleared, `?code=`
+      // stayed in the URL, attemptAutoLogin treated the page as a live callback
+      // forever, and the app sat on guardComponent with no way back — a reload
+      // just replayed the same failing callback.
+      vi.stubGlobal("console", { ...console, error: vi.fn() });
+      window.history.replaceState({}, "", "/?code=auth-code-123&state=xyz");
+
+      const mockLogin = vi.fn().mockResolvedValue(undefined);
+      const client = renderWithClient(
+        { getState: vi.fn(() => readyUnauthenticated), login: mockLogin },
+        vi.fn().mockRejectedValue(new Error("Missing session data")),
+      );
+
+      render(
+        <AuthProvider client={client} autoLogin={true}>
+          <div>Content</div>
+        </AuthProvider>,
+      );
+
+      await waitFor(() => {
+        expect(mockLogin).toHaveBeenCalledTimes(1);
+      });
+    });
+
+    it("stops re-initiating login once the callback retry budget is spent", async () => {
+      // Every retry is a full-page redirect to the authorization server, so a
+      // deterministic failure (evicted storage, misconfigured client) would loop
+      // forever without a ceiling.
+      vi.stubGlobal("console", { ...console, error: vi.fn() });
+      window.sessionStorage.setItem("tailor-app-shell:oauth-callback-failures", "1");
+      window.history.replaceState({}, "", "/?code=auth-code-123&state=xyz");
+
+      const mockLogin = vi.fn().mockResolvedValue(undefined);
+      const client = renderWithClient(
+        { getState: vi.fn(() => readyUnauthenticated), login: mockLogin },
+        vi.fn().mockRejectedValue(new Error("Missing session data")),
+      );
+
+      render(
+        <AuthProvider client={client} autoLogin={true}>
+          <div>Content</div>
+        </AuthProvider>,
+      );
+
+      await act(async () => {
+        await Promise.resolve();
+      });
+      await act(async () => {
+        await Promise.resolve();
+      });
+
+      expect(mockLogin).not.toHaveBeenCalled();
+    });
+
+    it("treats a denial that throws as denied rather than retryable", async () => {
+      // The server's `error` branch clears OAuth temp data first, which opens
+      // IndexedDB and can reject. The denial must not be reclassified as a
+      // retryable failure just because it arrived as a throw.
+      vi.stubGlobal("console", { ...console, error: vi.fn() });
+      window.history.replaceState({}, "", "/?error=access_denied");
+
+      const mockLogin = vi.fn().mockResolvedValue(undefined);
+      const client = renderWithClient(
+        { getState: vi.fn(() => readyUnauthenticated), login: mockLogin },
+        vi.fn().mockRejectedValue(new Error("storage unavailable")),
+      );
+
+      render(
+        <AuthProvider client={client} autoLogin={true}>
+          <div>Content</div>
+        </AuthProvider>,
+      );
+
+      await act(async () => {
+        await Promise.resolve();
+      });
+      await act(async () => {
+        await Promise.resolve();
+      });
+
+      expect(mockLogin).not.toHaveBeenCalled();
+      // Nothing was spent from the retry budget: this outcome is terminal.
+      expect(window.sessionStorage.getItem("tailor-app-shell:oauth-callback-failures")).toBeNull();
+    });
+
+    it("clears the retry budget once a callback succeeds", async () => {
+      window.sessionStorage.setItem("tailor-app-shell:oauth-callback-failures", "1");
+      window.history.replaceState({}, "", "/?code=auth-code-123&state=xyz");
+
+      const authenticated = { isAuthenticated: true, error: null, isReady: true };
+      renderWithClient(
+        { getState: vi.fn(() => authenticated) },
+        vi.fn().mockResolvedValue(undefined),
+      );
+
+      await waitFor(() => {
+        expect(
+          window.sessionStorage.getItem("tailor-app-shell:oauth-callback-failures"),
+        ).toBeNull();
+      });
+    });
+
+    it("lets a settled callback drive auto-login even if parameters remain in the URL", async () => {
+      // Isolates the narrowing in the auto-login gate: the URL is only an
+      // authority while the status is "idle". Once a callback has run, its
+      // status decides — reading the URL there is what stranded the app, since
+      // a failed callback used to leave its parameters in place permanently.
+      window.history.replaceState({}, "", "/?code=auth-code-123&state=xyz");
+
+      const mockLogin = vi.fn().mockResolvedValue(undefined);
+      const settledState = { isAuthenticated: false, error: "boom", isReady: true };
+      const mockClient = createSettledCallbackClient("rejected", {
+        login: mockLogin,
+        getState: vi.fn(() => settledState),
+      });
+
+      render(
+        <AuthProvider client={mockClient} autoLogin={true}>
+          <div>Content</div>
+        </AuthProvider>,
+      );
+
+      await waitFor(() => {
+        expect(mockLogin).toHaveBeenCalledTimes(1);
+      });
+    });
+
+    it("does not re-initiate login after the authorization server denies the callback", async () => {
+      // Recovering from a denial by redirecting straight back to the same
+      // authorization server would ask the same question and get the same
+      // answer — a redirect loop, which is worse than the dead end it replaced.
+      window.history.replaceState({}, "", "/?error=access_denied");
+
+      const mockLogin = vi.fn().mockResolvedValue(undefined);
+      // Stable reference: useSyncExternalStore loops if getSnapshot returns a
+      // fresh object each call.
+      const deniedState = { ...readyUnauthenticated, error: "access_denied" };
+      const client = renderWithClient(
+        { getState: vi.fn(() => deniedState), login: mockLogin },
+        vi.fn().mockResolvedValue(undefined),
+      );
+
+      render(
+        <AuthProvider client={client} autoLogin={true}>
+          <div>Content</div>
+        </AuthProvider>,
+      );
+
+      // Let the callback settle and every queued auto-login check run.
+      await act(async () => {
+        await Promise.resolve();
+      });
+      await act(async () => {
+        await Promise.resolve();
+      });
+
+      expect(mockLogin).not.toHaveBeenCalled();
+      // The parameters stay in the URL until upstream cleans failed callbacks
+      // too (auth-public-client#139) — correctness here rests on the status.
+      expect(window.location.search).toBe("?error=access_denied");
+    });
+  });
+
   it("does not call handleCallback when URL has no OAuth parameters", () => {
     // URL is already "/" from afterEach reset