fix(dashboard): prevent quick chat init race condition and infinite retry loop#45
Conversation
…etry loop Three interacting bugs caused "Failed to initialize chat" toast spam and prevented fresh session creation: 1. Race condition in handleCreateFreshSession — setting chatMode/ selectedAgentId before startFreshSession triggered the session-init useEffect, which called switchSession and resumed the old session instead of creating a new one. Fixed with skipNextSessionInitRef. 2. Infinite retry loop — shouldRetrySessionInit retried indefinitely when initializeSession failed, producing unbounded toast spam. Fixed with a 3-retry cap (initRetryCountRef). 3. Cascading re-renders — switchSession depended on activeSession in its closure, getting a new identity on every state change and re-triggering the consumer's useEffect. Fixed by reading activeSession from a ref instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a ref-based opt-out ( ChangesSession initialization & race prevention
sequenceDiagram
participant UI as QuickChatFAB (Component)
participant Hook as useQuickChat (Hook)
participant API as Chat API / initializeSession
participant Toast as Toast System
UI->>Hook: call useQuickChat(projectId, addToast)
UI->>Hook: invoke startFreshSession()
Hook->>Hook: set skipNextSessionInitRef.current = true\nreset initRetryCountRef
Hook->>API: create new session (explicit)
API-->>Hook: session created / error
alt success
Hook->>Hook: reset initRetryCountRef\nskipNextSessionInitRef.current = false
Hook->>UI: return active session
else failure
Hook->>Hook: initRetryCountRef++
alt within retries
Hook->>Toast: show error toast
end
Hook->>Hook: skipNextSessionInitRef.current = false
end
Note right of Hook: separate automatic init effect\nobserves skipNextSessionInitRef to avoid racing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR addresses three interacting bugs in
Confidence Score: 3/5The race condition and cascading re-render fixes are mechanically sound, but two prior review findings remain unaddressed: the shouldRetrySessionInit loop continues hitting the API without bound after the toast cap kicks in, and initRetryCountRef is not reset when switching to a new session target. The skip-flag and ref-mirror changes are correct and the synchronous flag placement before any awaits ensures the race window is closed. However, the retry-cap only gates toasts — the actual API loop driven by shouldRetrySessionInit in QuickChatFAB has no corresponding bail-out, so repeated network failures still hammer the endpoint indefinitely. Additionally, the retry counter persisting across target switches means a user who accumulates failures on agent A then switches to agent B will receive no error feedback for agent B failures. Both were flagged in earlier rounds and remain in the current code. packages/dashboard/app/hooks/useQuickChat.ts — the initRetryCountRef logic and its interaction with shouldRetrySessionInit in QuickChatFAB need a coordinated fix to stop unbounded API calls and properly scope the counter per session target. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant FAB as QuickChatFAB
participant Hook as useQuickChat
participant API as API
U->>FAB: Click "New Chat" (agent)
FAB->>FAB: setChatMode / setSelectedAgentId
FAB->>Hook: startFreshSession(agentId)
Note over Hook: skipNextSessionInitRef = true (sync)
Hook->>Hook: setActiveSession(null), setSessionsLoading(true)
Note over FAB: React flushes — session-init useEffect fires
FAB->>FAB: skipNextSessionInitRef.current? true → skip, record target as seen
Hook->>API: createSessionForTarget(target)
API-->>Hook: newSession
Hook->>Hook: setActiveSession(newSession)
Note over Hook: finally: skipNextSessionInitRef = false, setSessionsLoading(false)
Note over FAB: React flushes — useEffect fires again
FAB->>FAB: sessionTargetKey === prev, activeSession set → early return
Note over FAB,Hook: Normal switchSession path
FAB->>Hook: switchSession(agentId)
Hook->>Hook: isSameSession? reads activeSessionRef.current
alt New target
Hook->>Hook: setActiveSession(null)
Hook->>API: fetchResumeChatSession
API-->>Hook: session or null
Hook->>Hook: setActiveSession(result)
Note over Hook: initRetryCountRef++ on failure, toast if <= 3
else Same target
Hook->>API: fetchChatMessages (reload only)
end
Reviews (2): Last reviewed commit: "test(dashboard): add regression tests fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } 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); | ||
| } |
There was a problem hiding this comment.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/app/components/QuickChatFAB.tsx (1)
1209-1237:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRetry cap still doesn't stop the init loop.
shouldRetrySessionInitonly checks!activeSession && !sessionsLoading, so every failed init schedules anotherswitchSession/startModelChatas soon as loading flips back tofalse. The new counter only suppresses the toast; the backend request loop is still unbounded.🤖 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/components/QuickChatFAB.tsx` around lines 1209 - 1237, The retry logic lets failed inits loop because shouldRetrySessionInit only checks activeSession and sessionsLoading; add a retry-cap ref (e.g., sessionInitRetryRef) and MAX_SESSION_INIT_RETRIES constant, increment the ref each time you attempt startModelChat or switchSession, reset it when a session becomes active or when sessionTargetKey changes, and check sessionInitRetryRef.current < MAX_SESSION_INIT_RETRIES before invoking startModelChat/switchSession (and set prevSessionTargetRef.current to sessionTargetKey or flip skipNextSessionInitRef when cap reached) so no further backend requests are triggered after the max attempts; update the useEffect condition to include this retry-cap check alongside shouldRetrySessionInit.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/dashboard/app/hooks/useQuickChat.ts`:
- Around line 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.
---
Outside diff comments:
In `@packages/dashboard/app/components/QuickChatFAB.tsx`:
- Around line 1209-1237: The retry logic lets failed inits loop because
shouldRetrySessionInit only checks activeSession and sessionsLoading; add a
retry-cap ref (e.g., sessionInitRetryRef) and MAX_SESSION_INIT_RETRIES constant,
increment the ref each time you attempt startModelChat or switchSession, reset
it when a session becomes active or when sessionTargetKey changes, and check
sessionInitRetryRef.current < MAX_SESSION_INIT_RETRIES before invoking
startModelChat/switchSession (and set prevSessionTargetRef.current to
sessionTargetKey or flip skipNextSessionInitRef when cap reached) so no further
backend requests are triggered after the max attempts; update the useEffect
condition to include this retry-cap check alongside shouldRetrySessionInit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e991701e-cf84-499c-bde2-8a57bb524e0d
📒 Files selected for processing (2)
packages/dashboard/app/components/QuickChatFAB.tsxpackages/dashboard/app/hooks/useQuickChat.ts
| /** | ||
| * 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>; |
There was a problem hiding this comment.
🧩 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:
- 1: https://stackoverflow.com/questions/79300987/react-19-ref-as-prop-and-typescript
- 2: https://www.marcohaber.dev/blog/useref-evolution
- 3: https://medium.com/@ignatovich.dm/the-new-approach-to-passing-refs-in-react-19-typescript-a5762b938b93
- 4: https://dev.to/reactchallenges/new-way-of-passing-ref-to-child-in-react-19-22ja
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.
Six new tests covering the three bugs fixed in the previous commit: - Retry limit: stops showing toasts after 3 consecutive failures, resets counter on success - Skip flag: startFreshSession sets/clears skipNextSessionInitRef, creates new session even when existing one exists - Ref stability: switchSession reads activeSession from ref so it correctly detects "same session" and skips re-initialization Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/dashboard/app/hooks/__tests__/useQuickChat.test.ts (2)
1007-1024: ⚡ Quick winAdd a direct assertion that
switchSessioncallback identity stays stable.The comment explains identity stability as the core fix, but this test only checks API side effects. Add a
toBereference check before/afteractiveSessionupdates to lock the regression down.Proposed assertion diff
const { result } = renderHook(() => useQuickChat("proj-123")); + const initialSwitchSession = result.current.switchSession; await act(async () => { await result.current.switchSession("agent-001"); }); expect(result.current.activeSession).not.toBeNull(); + expect(result.current.switchSession).toBe(initialSwitchSession); @@ await act(async () => { await result.current.switchSession("agent-001"); }); + expect(result.current.switchSession).toBe(initialSwitchSession);🤖 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/__tests__/useQuickChat.test.ts` around lines 1007 - 1024, Capture the switchSession callback reference from result.current (e.g., const prevSwitch = result.current.switchSession) before calling switchSession again, then after awaiting the second switchSession call assert the identity is unchanged using expect(result.current.switchSession).toBe(prevSwitch); this locks the regression by verifying the callback's referential stability in addition to the existing API-call assertions (use the existing result.current.switchSession and activeSession-related setup in the test).
932-956: ⚡ Quick winThe skip-flag regression test never proves the flag is
truewhilestartFreshSessionis in flight.At Line 950-Line 952, the call is fully awaited, so only the final cleared state is observed. A regression where
skipNextSessionInitRefis never set would still pass.Proposed test-tightening diff
- // During startFreshSession — the flag is set then cleared in finally - await act(async () => { - await result.current.startFreshSession(); - }); + // Hold session creation to observe in-flight state + let resolveCreate: ((value: { session: ChatSession }) => void) | undefined; + mockCreateChatSession.mockImplementationOnce( + () => + new Promise<{ session: ChatSession }>((resolve) => { + resolveCreate = resolve; + }), + ); + + let freshPromise: Promise<void> | undefined; + act(() => { + freshPromise = result.current.startFreshSession(); + }); + + await waitFor(() => { + expect(result.current.skipNextSessionInitRef.current).toBe(true); + }); + + await act(async () => { + resolveCreate?.({ session: freshSession }); + await freshPromise; + });🤖 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/__tests__/useQuickChat.test.ts` around lines 932 - 956, The test doesn't observe skipNextSessionInitRef while startFreshSession is running because startFreshSession is fully awaited; modify the test in useQuickChat.test.ts to stub/mock the async call inside startFreshSession (e.g., mockCreateChatSession or whichever network call startFreshSession awaits) to return a controllable promise, call result.current.startFreshSession() without awaiting its resolution, assert that result.current.skipNextSessionInitRef.current is true while that promise is still pending, then resolve the promise and await completion and assert the flag is cleared and activeSession is "session-fresh"; reference startFreshSession, switchSession, skipNextSessionInitRef, and mockCreateChatSession (or the specific mocked fetch used by startFreshSession) to implement the controllable promise.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@packages/dashboard/app/hooks/__tests__/useQuickChat.test.ts`:
- Around line 1007-1024: Capture the switchSession callback reference from
result.current (e.g., const prevSwitch = result.current.switchSession) before
calling switchSession again, then after awaiting the second switchSession call
assert the identity is unchanged using
expect(result.current.switchSession).toBe(prevSwitch); this locks the regression
by verifying the callback's referential stability in addition to the existing
API-call assertions (use the existing result.current.switchSession and
activeSession-related setup in the test).
- Around line 932-956: The test doesn't observe skipNextSessionInitRef while
startFreshSession is running because startFreshSession is fully awaited; modify
the test in useQuickChat.test.ts to stub/mock the async call inside
startFreshSession (e.g., mockCreateChatSession or whichever network call
startFreshSession awaits) to return a controllable promise, call
result.current.startFreshSession() without awaiting its resolution, assert that
result.current.skipNextSessionInitRef.current is true while that promise is
still pending, then resolve the promise and await completion and assert the flag
is cleared and activeSession is "session-fresh"; reference startFreshSession,
switchSession, skipNextSessionInitRef, and mockCreateChatSession (or the
specific mocked fetch used by startFreshSession) to implement the controllable
promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 419a28dd-ebc1-4c09-a22f-5f3999a0dc8c
📒 Files selected for processing (1)
packages/dashboard/app/hooks/__tests__/useQuickChat.test.ts
2 participants
Summary
handleCreateFreshSessionwhere settingchatMode/selectedAgentIdbeforestartFreshSessiontriggered the session-init useEffect, causing it to resume the old session instead of creating a new oneinitRetryCountRef)switchSessiongot a new identity on everyactiveSessionchange, re-triggering the consumer's useEffect. Now readsactiveSessionfrom a refRoot Cause
Three interacting bugs in
useQuickChat/QuickChatFAB:Race condition:
handleCreateFreshSessioncalledsetChatMode("agent")+setSelectedAgentId(agentId)beforestartFreshSession(). These state changes triggered the session-init useEffect (line 1184), which calledswitchSession()→initializeSession()→fetchResumeChatSession()→ found and loaded the OLD session. The explicit fresh creation lost the race.Infinite retry loop: The
shouldRetrySessionInitcheck re-triggeredswitchSessionwheneveractiveSessionwas null andsessionsLoadingwas false. If the API call failed, this created an unbounded loop of "Failed to initialize chat" toasts.Cascading re-renders:
switchSessionhadactiveSessionin its dependency array, and the useEffect depended onswitchSession. EveryactiveSessionchange → newswitchSessionidentity → useEffect re-runs.Test plan
useQuickChat+ 19QuickChatFAB)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests