Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gsxdsm merged 2 commits into from
May 6, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e5b400
fix(dashboard): prevent quick chat init race condition and infinite r…
timothyjlaurent May 5, 2026
2746015
test(dashboard): add regression tests for quick chat init fixes
timothyjlaurent May 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/dashboard/app/components/QuickChatFAB.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,6 +954,7 @@ export function QuickChatFAB({
startModelChat,
startFreshSession,
refreshSessions,
skipNextSessionInitRef,
} = useQuickChat(projectId, addToast);

const panelRef = useRef<HTMLDivElement | null>(null);
Expand Down Expand Up @@ -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 = "";
Expand All @@ -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;
Expand Down Expand Up @@ -1220,6 +1234,7 @@ export function QuickChatFAB({
sessionsLoading,
startModelChat,
switchSession,
skipNextSessionInitRef,
]);

useEffect(() => {
Expand Down
198 changes: 198 additions & 0 deletions packages/dashboard/app/hooks/__tests__/useQuickChat.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down
52 changes: 49 additions & 3 deletions packages/dashboard/app/hooks/useQuickChat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Comment on lines +62 to +68
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

In React 19 / @types/react19, isMutableRefObjectstill exported foruseRef, or should RefObject be used instead?

💡 Result:

In React 19 with @types/react 19, MutableRefObject is no longer the type you should rely on for useRef. useRef is typed to return RefObject<T> (and “all refs are mutable” in the React 19 type model), so you should use RefObject<T> (or the more general Ref<T>/React.Ref<T> when typing the ref prop) instead of MutableRefObject<T> for new code.[1][2][3] Practical guidance: 1) For your own useRef variable: prefer const x = useRef<T>(...) and let TypeScript infer RefObject<T>; don’t annotate as MutableRefObject<T>.[1][2] 2) For a component prop that receives a ref (React 19 “ref as prop”): type it as ref?: React.Ref<T> (or ref?: React.RefObject<T> if you specifically want an object ref). Using React.Ref<T> is the generally correct choice for a prop that may receive either callback refs or object refs.[4] Answering your question directly: MutableRefObject may still exist for backwards compatibility, but in the React 19 type system it’s deprecated/merged away conceptually—useRef returns RefObject, so you should use RefObject<T> rather than MutableRefObject<T>.[1][2]

Citations:

Change React.MutableRefObject<boolean> to React.RefObject<boolean>

React 19's TypeScript type system no longer uses MutableRefObject for useRef returns; it uses RefObject<T> instead. Update the public API signature from skipNextSessionInitRef: React.MutableRefObject<boolean> to skipNextSessionInitRef: React.RefObject<boolean>.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/dashboard/app/hooks/useQuickChat.ts` around lines 62 - 68, Update
the public API type for skipNextSessionInitRef from
React.MutableRefObject<boolean> to React.RefObject<boolean> in the useQuickChat
hook and any exported types/interfaces that reference it (e.g., the useQuickChat
return type or related props), ensuring the signature now reads
skipNextSessionInitRef: React.RefObject<boolean> so it matches React 19's useRef
typing; adjust any import/exports or type aliases that reference
skipNextSessionInitRef to keep types consistent.

}

function normalizeModelSelection(modelProvider?: string, modelId?: string): ModelSelection {
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -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);
}
Comment on lines 308 to 319
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Retry cap silences toasts but not API calls

initRetryCountRef guards the addToast call, but shouldRetrySessionInit in QuickChatFAB has no corresponding guard. After 3 failures activeSession is still null and sessionsLoading goes false, so the useEffect keeps triggering switchSession → initializeSession in an unbounded loop — just without user-visible toasts. The fix prevents toast spam but leaves the actual infinite API call loop in place. initializeSession should bail early when initRetryCountRef.current >= INIT_MAX_RETRIES, or the component's shouldRetrySessionInit condition must receive an exposed ref to gate retries entirely.

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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;
Expand All @@ -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]);
Expand Down Expand Up @@ -656,6 +701,7 @@ export function useQuickChat(
refreshSessions,
loadMessages,
reloadMessages,
skipNextSessionInitRef,
}), [
activeSession,
sessions,
Expand Down
Loading