fix: assistant: clear saved state on sign-out, and strip ?sessionId= after a failed open (#1999)

packages/shared/providers/authentication/provider.tsx

@@ -79,6 +79,9 @@ export function AuthProvider({
   }, []);
 
   const logout = useCallback(async () => {
+    // Signed out on the page first: anything reading the account stops now,
+    // rather than sending one more request against a session being ended.
+    setUser(null);
     try {
       await fetch(`${API_BASE_URL}/auth/logout`, {
         credentials: "include",
@@ -87,7 +90,6 @@ export function AuthProvider({
     } catch {
       // Best-effort logout.
     }
-    setUser(null);
   }, []);
 
   return (

packages/shared/views/AssistantView/hooks/UseAssistantChat/hook.ts

@@ -146,7 +146,17 @@ export const useAssistantChat = ({
     setSaveRetryTick((tick) => tick + 1);
   }, []);
   const router = useRouter();
+  // The router as of the latest render, for work that settles later: the one
+  // a callback closed over can predate a URL write made since.
+  const routerRef = useRef(router);
+  useEffect(() => {
+    routerRef.current = router;
+  });
   const { isAuthenticated, isConfigured, isLoading: isAuthLoading } = useAuth();
+  // Whether the conversation is saved, and to which analysis, is the account's
+  // to say. Read off auth, so a save, reply or restore that lands after signing
+  // out can't put either back.
+  const isSignedOut = isConfigured && !isAuthLoading && !isAuthenticated;
   // A question of whitespace is no question: it would neither be asked nor
   // leave the conversation it displaced restorable.
   const question = initialMessage?.trim();
@@ -237,6 +247,12 @@ export const useAssistantChat = ({
           // localStorage pointer going bad is routine.
           if (isNamed) {
             setError("That conversation is no longer available.");
+            // A named session came from ?sessionId=, or an open wrote it there.
+            // The param outranks the stored pointer on mount, so left behind it
+            // would fail this way on every reload.
+            stripQueryParam(routerRef.current, [
+              ASSISTANT_QUERY_PARAM.SESSION_ID,
+            ]);
           }
         })
         .finally(() => {
@@ -349,8 +365,7 @@ export const useAssistantChat = ({
         // Checked against the session, not this effect run: a turn or an open
         // starting re-runs the effect, though the conversation is still the
         // one just written, and one that fails would never say so. A
-        // conversation that replaced it has moved the session on, and signing
-        // out has dropped the latch.
+        // conversation that replaced it has moved the session on.
         if (
           sessionIdRef.current === currentSessionId &&
           saveAttemptRef.current === currentSessionId
@@ -392,21 +407,6 @@ export const useAssistantChat = ({
 
   useEffect(() => (): void => clearSaveRetry(saveRetryTimerRef), []);
 
-  // Signing out doesn't unsave anything server-side, but it ends this
-  // browser's claim to the conversation: AuthProvider.logout only clears the
-  // user, with no reload, so the panel was left rendering "Saved to your
-  // account" and "Sign in to keep this conversation" side by side. The latch
-  // goes too, so a save still in flight no longer counts once it lands; signing
-  // back in is a full-page redirect, so it never meets this hook's refs.
-  useEffect(() => {
-    if (isAuthLoading || !isConfigured || isAuthenticated) return;
-    saveAttemptRef.current = null;
-    saveRetriesRef.current = { count: 0, sessionId: null };
-    clearSaveRetry(saveRetryTimerRef);
-    // eslint-disable-next-line react-hooks/set-state-in-effect -- react-hooks v7 anti-pattern (setState in effect)
-    setIsSaved(false);
-  }, [isAuthLoading, isAuthenticated, isConfigured]);
-
   // One path for every turn. A setup-panel clear rides it too, as structured
   // clear_fields the server applies before the model runs, so the clear can't
   // hinge on how the model reads a message and the assistant still answers it.
@@ -642,12 +642,12 @@ export const useAssistantChat = ({
       error,
       handoffUrl,
       isRestoring,
-      isSaved,
+      isSaved: isSaved && !isSignedOut,
       lastSave,
       loading,
       logan,
       messages,
-      savedAnalysisId,
+      savedAnalysisId: isSignedOut ? null : savedAnalysisId,
       schema,
       shownSessionId,
       suggestions,

packages/shared/views/AssistantView/hooks/UseAssistantChat/types.ts

@@ -78,11 +78,13 @@ export interface UseAssistantChatState {
   handoffUrl: string | null;
   /** A conversation is being restored, or a saved one opened. */
   isRestoring: boolean;
+  /** The conversation is saved to the signed-in account; false once signed out. */
   isSaved: boolean;
   lastSave: LastSave | null;
   loading: boolean;
   logan: LoganContext | null;
   messages: ChatMessageDisplay[];
+  /** Saved analysis the conversation belongs to; null while it has none or once signed out. */
   savedAnalysisId: string | null;
   schema: AnalysisSchema | null;
   shownSessionId: string | null;

tests/hooks/useAssistantChat.autosave.test.ts

@@ -551,6 +551,92 @@ describe("useAssistantChat auto-save", () => {
     expect(result.current.state.isSaved).toBe(false);
   });
 
+  test("signing out leaves no saved analysis showing as open", async () => {
+    auth(true);
+    mockClient.assistantRestore.mockResolvedValue({
+      ...restored(),
+      saved: true,
+      saved_analysis_id: "analysis-1",
+    } as unknown as Awaited<ReturnType<typeof mockClient.assistantRestore>>);
+
+    const { rerender, result } = renderHook(() =>
+      useAssistantChat({ sessionKey: SESSION_KEY })
+    );
+    await waitFor(() =>
+      expect(result.current.state.savedAnalysisId).toBe("analysis-1")
+    );
+
+    auth(false);
+    rerender();
+
+    await waitFor(() =>
+      expect(result.current.state.savedAnalysisId).toBeNull()
+    );
+  });
+
+  test("a conversation that loads after signing out does not show as saved", async () => {
+    auth(true);
+    const restore =
+      deferred<Awaited<ReturnType<typeof mockClient.assistantRestore>>>();
+    mockClient.assistantRestore.mockReturnValueOnce(restore.promise);
+
+    const { rerender, result } = renderHook(() =>
+      useAssistantChat({ sessionKey: SESSION_KEY })
+    );
+    await waitFor(() =>
+      expect(mockClient.assistantRestore).toHaveBeenCalledWith(STORED_ID)
+    );
+
+    auth(false);
+    rerender();
+    await act(async () => {
+      restore.resolve({
+        ...restored(),
+        saved: true,
+        saved_analysis_id: "analysis-1",
+      } as unknown as Awaited<ReturnType<typeof mockClient.assistantRestore>>);
+    });
+
+    expect(result.current.state.messages).toHaveLength(2);
+    expect(result.current.state.isSaved).toBe(false);
+    expect(result.current.state.savedAnalysisId).toBeNull();
+  });
+
+  test("a reply that lands after signing out does not show as saved", async () => {
+    auth(true);
+    mockClient.assistantRestore.mockResolvedValue({
+      ...restored(),
+      saved: true,
+    } as unknown as Awaited<ReturnType<typeof mockClient.assistantRestore>>);
+    const turn =
+      deferred<Awaited<ReturnType<typeof mockClient.assistantChat>>>();
+    mockClient.assistantChat.mockReturnValueOnce(turn.promise);
+
+    const { rerender, result } = renderHook(() =>
+      useAssistantChat({ sessionKey: SESSION_KEY })
+    );
+    await waitFor(() => expect(result.current.state.isSaved).toBe(true));
+    let sending!: Promise<void>;
+    act(() => {
+      sending = result.current.actions.onSendMessage("still there?");
+    });
+
+    auth(false);
+    rerender();
+    await act(async () => {
+      turn.resolve({
+        ...chatReply(),
+        saved: true,
+        saved_analysis_id: "analysis-1",
+      });
+      await sending;
+    });
+
+    expect(result.current.state.messages.at(-1)?.content).toBe("ok");
+    expect(result.current.state.isSaved).toBe(false);
+    expect(result.current.state.savedAnalysisId).toBeNull();
+  });
+
   test("a reply that moves to a new, unsaved session does not keep the saved label", async () => {
     auth(true);
     const save = deferred<SaveResponse>();

tests/hooks/useAssistantChat.restore.test.ts

@@ -90,6 +90,8 @@ describe("useAssistantChat restore", () => {
     await waitFor(() => expect(result.current.state.isRestoring).toBe(false));
     expect(localStorage.getItem(SESSION_KEY)).toBeNull();
     expect(result.current.state.error).toBeNull();
+    // The session came from storage, not the URL, so there is no URL to fix.
+    expect(mockReplace).not.toHaveBeenCalled();
   });
 
   test("a 404 on a session named in the URL says so rather than rendering blank", async () => {
@@ -103,6 +105,24 @@ describe("useAssistantChat restore", () => {
     expect(result.current.state.error).toMatch(/no longer available/i);
   });
 
+  test("a 404 on a session named in the URL drops it from the URL", async () => {
+    // ?sessionId= outranks the stored pointer on mount, so left behind it fails
+    // the same way on every reload.
+    mockQuery = { sessionId: STORED_ID };
+    mockClient.assistantRestore.mockRejectedValue(httpError(404));
+
+    const { result } = renderHook(() =>
+      useAssistantChat({ initialSessionId: STORED_ID, sessionKey: SESSION_KEY })
+    );
+
+    await waitFor(() => expect(result.current.state.isRestoring).toBe(false));
+    expect(mockReplace).toHaveBeenLastCalledWith(
+      { pathname: "/assistant", query: {} },
+      undefined,
+      { shallow: true }
+    );
+  });
+
   test("a 403 drops the pointer -- this browser can never restore that session", async () => {
     // The signing cookie no longer matches the id, which is permanent. Keeping
     // it would re-send the same id on every message and wedge the chat.
@@ -623,6 +643,37 @@ describe("useAssistantChat opening a saved conversation", () => {
     expect(result.current.state.error).toBeNull();
   });
 
+  test("a conversation refused after an open is dropped from the URL", async () => {
+    const { result } = await renderRestored();
+    mockClient.assistantRestore.mockRejectedValueOnce(httpError(404));
+
+    await openConversation(result, OPENED_ID);
+    await waitFor(() => expect(result.current.state.isRestoring).toBe(false));
+
+    expect(result.current.state.error).toMatch(/no longer available/i);
+    expect(mockReplace).toHaveBeenLastCalledWith(
+      { pathname: "/assistant", query: {} },
+      undefined,
+      { shallow: true }
+    );
+  });
+
+  test("a conversation that fails to load after an open for a transient reason stays in the URL", async () => {
+    // It is probably still there, and a reload is how it comes back.
+    const { result } = await renderRestored();
+    mockClient.assistantRestore.mockRejectedValueOnce(httpError(503));
+
+    await openConversation(result, OPENED_ID);
+    await waitFor(() => expect(result.current.state.isRestoring).toBe(false));
+
+    expect(mockReplace).toHaveBeenCalledTimes(1);
+    expect(mockReplace).toHaveBeenLastCalledWith(
+      { pathname: "/assistant", query: { sessionId: OPENED_ID } },
+      undefined,
+      { shallow: true }
+    );
+  });
+
   test("a URL naming another session after mount restores nothing", async () => {
     // Only the mount restores from the URL; an open writes the URL as its
     // output, and must not be restored a second time from it.

tests/providers/authenticationProvider.test.tsx

@@ -0,0 +1,62 @@
+import {
+  AuthProvider,
+  useAuth,
+} from "@repo/shared/providers/authentication/provider";
+import { act, renderHook, waitFor } from "@testing-library/react";
+import type { JSX, ReactNode } from "react";
+
+const USER = {
+  email: "user@example.org",
+  name: "User",
+  preferred_username: "user",
+  sub: "user-1",
+};
+
+/**
+ * Wraps a hook in a provider with login turned on.
+ * @param props - Component props.
+ * @param props.children - The hook under test.
+ * @returns The provider around the hook.
+ */
+function Wrapper({ children }: { children: ReactNode }): JSX.Element {
+  return <AuthProvider loginEnabled>{children}</AuthProvider>;
+}
+
+describe("AuthProvider logout", () => {
+  let logoutRequest: { resolve: () => void };
+
+  beforeEach(() => {
+    global.fetch = jest.fn((url: RequestInfo | URL) => {
+      if (String(url).endsWith("/auth/me")) {
+        return Promise.resolve({
+          json: () => Promise.resolve(USER),
+          ok: true,
+        } as Response);
+      }
+      return new Promise<Response>((resolve) => {
+        logoutRequest = {
+          resolve: (): void => resolve({ ok: true } as Response),
+        };
+      });
+    }) as jest.Mock;
+  });
+
+  test("signs out on the page before the server has ended the session", async () => {
+    // Anything reading the account while the logout request is in flight would
+    // otherwise send one more request against a session being ended.
+    const { result } = renderHook(() => useAuth(), { wrapper: Wrapper });
+    await waitFor(() => expect(result.current.isAuthenticated).toBe(true));
+
+    let loggingOut!: Promise<void>;
+    act(() => {
+      loggingOut = result.current.logout();
+    });
+
+    expect(result.current.isAuthenticated).toBe(false);
+    await act(async () => {
+      logoutRequest.resolve();
+      await loggingOut;
+    });
+    expect(result.current.isAuthenticated).toBe(false);
+  });
+});