Merge pull request #45 from timothyjlaurent/fix/quick-chat-init-race-condition

fix(dashboard): prevent quick chat init race condition and infinite retry loop

packages/dashboard/app/components/QuickChatFAB.tsx

@@ -954,6 +954,7 @@ export function QuickChatFAB({
     startModelChat,
     startFreshSession,
     refreshSessions,
+    skipNextSessionInitRef,
   } = useQuickChat(projectId, addToast);
 
   const panelRef = useRef<HTMLDivElement | null>(null);
@@ -1181,6 +1182,10 @@ export function QuickChatFAB({
   }, [isOpen, refreshSessions]);
 
   // Initialize/switch quick chat session whenever the selected target changes.
+  // NOTE: activeSession and sessionsLoading are in the dependency array to
+  // enable retry-when-null (see shouldRetrySessionInit), but the hook's
+  // switchSession now reads activeSession from a ref so it doesn't get a
+  // new identity on every activeSession change.
   useEffect(() => {
     if (!isOpen) {
       prevSessionTargetRef.current = "";
@@ -1192,6 +1197,15 @@ export function QuickChatFAB({
       return;
     }
 
+    // When startFreshSession is in progress, skip the automatic init to
+    // prevent racing with the explicit fresh-session creation.  Record the
+    // target key as "seen" so a later render won't re-trigger for the same
+    // target.
+    if (skipNextSessionInitRef.current) {
+      prevSessionTargetRef.current = sessionTargetKey;
+      return;
+    }
+
     const shouldRetrySessionInit = sessionTargetKey === prevSessionTargetRef.current
       && !activeSession
       && !sessionsLoading;
@@ -1220,6 +1234,7 @@ export function QuickChatFAB({
     sessionsLoading,
     startModelChat,
     switchSession,
+    skipNextSessionInitRef,
   ]);
 
   useEffect(() => {

packages/dashboard/app/hooks/__tests__/useQuickChat.test.ts

@@ -856,6 +856,204 @@ describe("useQuickChat", () => {
     });
   });
 
+  describe("regression: init retry limit prevents infinite toast spam", () => {
+    it("stops showing error toasts after 3 consecutive initialization failures", async () => {
+      const addToast = vi.fn();
+      mockFetchResumeChatSession.mockRejectedValue(new Error("API down"));
+
+      const { result } = renderHook(() => useQuickChat("proj-123", addToast));
+
+      // Attempt 1
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+      // Attempt 2
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+      // Attempt 3
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+      // Attempt 4 — should be silently dropped
+      await act(async () => {
+        await result.current.switchSession("agent-002");
+      });
+
+      // Should have exactly 3 toast calls (one per unique target up to the limit)
+      // Each new target resets the retry counter, so agent-001 gets 3 toasts
+      // and agent-002 starts fresh. But agent-001 only has 3 switchSession calls
+      // each with a different internal retry.
+      //
+      // Actually: switchSession clears the session each time, so each call
+      // goes through initializeSession which increments the counter.
+      // The counter resets on success or when startFreshSession is called.
+      // With 3 calls for agent-001, that's 3 failures = 3 toasts.
+      // Then agent-002 starts a new sequence — retry counter continues
+      // because it's the same hook instance.
+      expect(addToast).toHaveBeenCalledTimes(3);
+      expect(addToast).toHaveBeenNthCalledWith(1, "Failed to initialize chat", "error");
+      expect(addToast).toHaveBeenNthCalledWith(2, "Failed to initialize chat", "error");
+      expect(addToast).toHaveBeenNthCalledWith(3, "Failed to initialize chat", "error");
+    });
+
+    it("resets retry counter after a successful initialization", async () => {
+      const addToast = vi.fn();
+
+      const session = makeSession({ id: "session-001", agentId: "agent-001" });
+      mockFetchResumeChatSession
+        .mockRejectedValueOnce(new Error("API down"))
+        .mockResolvedValueOnce({ session });
+
+      const { result } = renderHook(() => useQuickChat("proj-123", addToast));
+
+      // Fail once
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+      expect(addToast).toHaveBeenCalledTimes(1);
+
+      // Succeed — resets counter
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+      expect(result.current.activeSession?.id).toBe("session-001");
+
+      // Fail again — counter was reset, so toast shows again
+      mockFetchResumeChatSession.mockRejectedValue(new Error("API down again"));
+      await act(async () => {
+        await result.current.switchSession("agent-002");
+      });
+      expect(addToast).toHaveBeenCalledTimes(2);
+    });
+  });
+
+  describe("regression: startFreshSession skip flag prevents useEffect race", () => {
+    it("sets skipNextSessionInitRef during startFreshSession and clears it after", async () => {
+      const existingSession = makeSession({ id: "session-existing", agentId: "agent-001" });
+      const freshSession = makeSession({ id: "session-fresh", agentId: "agent-001" });
+
+      mockFetchResumeChatSession.mockResolvedValueOnce({ session: existingSession });
+      mockCreateChatSession.mockResolvedValueOnce({ session: freshSession });
+      mockFetchChatMessages.mockResolvedValue({ messages: [] });
+
+      const { result } = renderHook(() => useQuickChat("proj-123"));
+
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+
+      // Before startFreshSession — flag should be false
+      expect(result.current.skipNextSessionInitRef.current).toBe(false);
+
+      // During startFreshSession — the flag is set then cleared in finally
+      await act(async () => {
+        await result.current.startFreshSession();
+      });
+
+      // After startFreshSession — flag should be cleared
+      expect(result.current.skipNextSessionInitRef.current).toBe(false);
+      expect(result.current.activeSession?.id).toBe("session-fresh");
+    });
+
+    it("startFreshSession creates a new session even when an existing one exists for the same agent", async () => {
+      const existingSession = makeSession({ id: "session-old", agentId: "agent-001" });
+      const freshSession = makeSession({ id: "session-new", agentId: "agent-001" });
+
+      mockFetchResumeChatSession.mockResolvedValueOnce({ session: existingSession });
+      mockCreateChatSession.mockResolvedValueOnce({ session: freshSession });
+      mockFetchChatMessages.mockResolvedValue({ messages: [] });
+
+      const { result } = renderHook(() => useQuickChat("proj-123"));
+
+      // First switchSession resumes existing session
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+      expect(result.current.activeSession?.id).toBe("session-old");
+
+      // startFreshSession should bypass resume and create a new one
+      await act(async () => {
+        await result.current.startFreshSession();
+      });
+
+      await waitFor(() => {
+        expect(result.current.activeSession?.id).toBe("session-new");
+      });
+
+      // createSession was called (not resume)
+      expect(mockCreateChatSession).toHaveBeenCalledWith(
+        { agentId: "agent-001" },
+        "proj-123",
+      );
+    });
+  });
+
+  describe("regression: switchSession uses activeSessionRef to avoid cascading re-renders", () => {
+    it("switchSession does not re-initialize when activeSession changes", async () => {
+      const sessionA = makeSession({ id: "session-a", agentId: "agent-001" });
+
+      mockFetchResumeChatSession.mockResolvedValue({ session: sessionA });
+      mockFetchChatMessages.mockResolvedValue({ messages: [] });
+
+      const { result } = renderHook(() => useQuickChat("proj-123"));
+
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+
+      expect(result.current.activeSession).not.toBeNull();
+
+      // Calling switchSession again with the same target should NOT
+      // go through initializeSession again — it should just reload
+      // messages (because activeSessionRef.current is set, making
+      // isSameSession = true). This is the key behavioral fix:
+      // even though switchSession gets a new identity from other deps,
+      // it reads activeSession from a ref so it correctly detects
+      // "same session" and skips re-initialization.
+      const resumeCallCount = mockFetchResumeChatSession.mock.calls.length;
+      const createCallCount = mockCreateChatSession.mock.calls.length;
+
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+
+      // No additional API calls for session lookup or creation
+      expect(mockFetchResumeChatSession.mock.calls.length).toBe(resumeCallCount);
+      expect(mockCreateChatSession.mock.calls.length).toBe(createCallCount);
+
+      // Messages were reloaded though
+      expect(mockFetchChatMessages).toHaveBeenCalled();
+    });
+
+    it("switchSession with same target after activeSession change just reloads messages", async () => {
+      const sessionA = makeSession({ id: "session-a", agentId: "agent-001" });
+
+      mockFetchResumeChatSession.mockResolvedValue({ session: sessionA });
+      mockFetchChatMessages.mockResolvedValue({ messages: [] });
+
+      const { result } = renderHook(() => useQuickChat("proj-123"));
+
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+
+      expect(result.current.activeSession?.id).toBe("session-a");
+
+      // Calling switchSession again with same target should reload messages,
+      // not call fetchResumeChatSession again
+      const initialResumeCallCount = mockFetchResumeChatSession.mock.calls.length;
+
+      await act(async () => {
+        await result.current.switchSession("agent-001");
+      });
+
+      // No additional resume lookup — just message reload
+      expect(mockFetchResumeChatSession.mock.calls.length).toBe(initialResumeCallCount);
+      expect(mockFetchChatMessages).toHaveBeenCalled();
+    });
+  });
+
   describe("FN-3336: streaming state recovery on reload", () => {
     it("sets isStreaming=true when initializing a session with isGenerating=true", async () => {
       const session = { ...makeSession({ id: "session-001", agentId: "agent-001" }), isGenerating: true };

packages/dashboard/app/hooks/useQuickChat.ts

@@ -58,6 +58,14 @@ export interface UseQuickChatReturn {
   refreshSessions: () => Promise<void>;
   loadMessages: () => Promise<void>;
   reloadMessages: () => Promise<void>;
+
+  /**
+   * When true, the consuming component's session-init useEffect should
+   * skip its automatic switchSession call.  Set during startFreshSession
+   * to prevent the useEffect from racing with an explicit fresh-session
+   * creation.
+   */
+  skipNextSessionInitRef: React.MutableRefObject<boolean>;
 }
 
 function normalizeModelSelection(modelProvider?: string, modelId?: string): ModelSelection {
@@ -208,6 +216,24 @@ export function useQuickChat(
   const currentSessionKeyRef = useRef<string>("");
   const currentSessionTargetRef = useRef<SessionTarget | null>(null);
 
+  // Ref mirror of activeSession to avoid cascading re-renders through
+  // switchSession's dependency array.  Reading activeSession from the
+  // closure causes switchSession to get a new identity every time
+  // activeSession changes — which then re-triggers the consuming
+  // component's useEffect that depends on switchSession.
+  const activeSessionRef = useRef<ChatSession | null>(activeSession);
+  activeSessionRef.current = activeSession;
+
+  // Max retries for session init to prevent infinite toast loops
+  const initRetryCountRef = useRef(0);
+  const INIT_MAX_RETRIES = 3;
+
+  // When true, the consuming component's session-init useEffect should
+  // skip its switchSession call.  Set by startFreshSession (and the
+  // component's handleCreateFreshSession) to prevent the automatic
+  // useEffect from racing with an explicit fresh-session creation.
+  const skipNextSessionInitRef = useRef(false);
+
   useEffect(() => {
     pendingMessageRef.current = pendingMessage;
   }, [pendingMessage]);
@@ -276,9 +302,18 @@ export function useQuickChat(
           setActiveSession(newSession);
           currentSessionKeyRef.current = sessionKey;
         }
+
+        // Reset retry counter on success so a later failure can retry again
+        initRetryCountRef.current = 0;
       } catch (err) {
         console.error("[useQuickChat] Failed to initialize session:", err);
-        addToast?.("Failed to initialize chat", "error");
+        // Only show the toast while under the retry limit — once the limit
+        // is reached the user has already seen the warning and further
+        // toasts just create noise.
+        initRetryCountRef.current += 1;
+        if (initRetryCountRef.current <= INIT_MAX_RETRIES) {
+          addToast?.("Failed to initialize chat", "error");
+        }
       } finally {
         setSessionsLoading(false);
       }
@@ -379,7 +414,10 @@ export function useQuickChat(
       const targetSessionKey = buildSessionKey(target.agentId, target.modelProvider, target.modelId);
       currentSessionTargetRef.current = target;
 
-      const isSameSession = targetSessionKey === currentSessionKeyRef.current && activeSession;
+      // Use ref to avoid cascading re-renders: reading activeSession from
+      // the closure would make switchSession change identity every time
+      // activeSession changes, triggering the consumer's useEffect again.
+      const isSameSession = targetSessionKey === currentSessionKeyRef.current && activeSessionRef.current;
 
       if (!isSameSession) {
         // Close any existing stream
@@ -407,7 +445,7 @@ export function useQuickChat(
       currentSessionKeyRef.current = targetSessionKey;
       await initializeSession(target.agentId, target.modelProvider, target.modelId);
     },
-    [activeSession, initializeSession, reloadMessages, resetTransientComposerState],
+    [initializeSession, reloadMessages, resetTransientComposerState],
   );
 
   const selectSession = useCallback(async (session: ChatSession) => {
@@ -444,6 +482,12 @@ export function useQuickChat(
     // This preserves normal switchSession resume behavior while allowing multiple threads per target.
     const targetSessionKey = buildSessionKey(target.agentId, target.modelProvider, target.modelId);
 
+    // Prevent the consuming component's automatic session-init useEffect
+    // from racing with this explicit fresh-session creation.  The effect
+    // will see the flag, record the target key as "seen", and skip.
+    skipNextSessionInitRef.current = true;
+    initRetryCountRef.current = 0;
+
     if (streamRef.current) {
       streamRef.current.close();
       streamRef.current = null;
@@ -465,6 +509,7 @@ export function useQuickChat(
       console.error("[useQuickChat] Failed to start a fresh session:", err);
       addToast?.("Failed to start a new chat", "error");
     } finally {
+      skipNextSessionInitRef.current = false;
       setSessionsLoading(false);
     }
   }, [addToast, createSessionForTarget, projectId, resetTransientComposerState]);
@@ -656,6 +701,7 @@ export function useQuickChat(
     refreshSessions,
     loadMessages,
     reloadMessages,
+    skipNextSessionInitRef,
   }), [
     activeSession,
     sessions,