fix(frontend): Ask spinner never stops under React Strict Mode - #1235
fix(frontend): Ask spinner never stops under React Strict Mode#1235zivkidd1 wants to merge 2 commits into
Conversation
The mountedRef effect only declared a cleanup function and never set the ref back to true on mount. React Strict Mode mounts, unmounts and remounts in development, so the first cleanup left mountedRef.current false forever and every guard depending on it became dead code. That silently disabled all five paths that end the loading state: stopStreaming on final_answer and complete, the normal end-of-stream call, the error handler, and the idle watchdog added in lfnovo#777. The Ask spinner ran forever and the answer was never rendered, even when the stream completed successfully.
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 4/5
- In
frontend/src/lib/hooks/use-ask.ts, the code change looks minimal, but without a regression test for the Strict Mode mount/unmount behavior this bug could silently return in a later refactor and reintroduce duplicate or incorrect ask flows for users—add a focused regression test that reproduces the original Strict Mode scenario and captures the before/after evidence.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/lib/hooks/use-ask.ts">
<violation number="1" location="frontend/src/lib/hooks/use-ask.ts:58">
P3: The Strict Mode fix itself is correct and minimal. Since this is framed as a bug fix and the project convention requires a regression test with real evidence for bug fixes, consider adding a test that exercises the mount → unmount → remount sequence and asserts that the mountedRef guards still allow the end-of-loading paths (stopStreaming on final_answer/complete, idle watchdog, error clear of isStreaming) to run. The current PR only notes that the existing 140-test suite still passes, which cannot catch this dev-only regression.</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| // Must be re-set on every mount: React Strict Mode mounts, unmounts and | ||
| // remounts in development, and the cleanup below would otherwise leave this | ||
| // false forever, making every mountedRef guard dead code. | ||
| mountedRef.current = true |
There was a problem hiding this comment.
P3: The Strict Mode fix itself is correct and minimal. Since this is framed as a bug fix and the project convention requires a regression test with real evidence for bug fixes, consider adding a test that exercises the mount → unmount → remount sequence and asserts that the mountedRef guards still allow the end-of-loading paths (stopStreaming on final_answer/complete, idle watchdog, error clear of isStreaming) to run. The current PR only notes that the existing 140-test suite still passes, which cannot catch this dev-only regression.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/lib/hooks/use-ask.ts, line 58:
<comment>The Strict Mode fix itself is correct and minimal. Since this is framed as a bug fix and the project convention requires a regression test with real evidence for bug fixes, consider adding a test that exercises the mount → unmount → remount sequence and asserts that the mountedRef guards still allow the end-of-loading paths (stopStreaming on final_answer/complete, idle watchdog, error clear of isStreaming) to run. The current PR only notes that the existing 140-test suite still passes, which cannot catch this dev-only regression.</comment>
<file context>
@@ -52,6 +52,10 @@ export function useAsk() {
+ // Must be re-set on every mount: React Strict Mode mounts, unmounts and
+ // remounts in development, and the cleanup below would otherwise leave this
+ // false forever, making every mountedRef guard dead code.
+ mountedRef.current = true
return () => {
mountedRef.current = false
</file context>
Renders useAsk inside StrictMode so the mount → unmount → remount sequence runs, then drives a mocked SSE stream to completion and to an in-band error event. Both assertions fail on the pre-fix hook with `expected true to be false`, because mountedRef.current stays false and every setState becomes a no-op — the existing suite could not catch this since nothing exercised a remount.
Problem
In development the Ask panel spins forever and the answer is never rendered — even when the SSE stream completes normally and the backend returns
200.Root cause
useAsk's mount-tracking effect declares only a cleanup function and never sets the ref back totrue:reactStrictModedefaults totrue(Next.js 13.5+), so in development React mounts → unmounts → remounts. The first cleanup setsmountedRef.current = falseand nothing ever sets it back, so everyif (mountedRef.current)guard in the hook is dead code for the rest of the session.That silently disables all five paths that end the loading state, since each one updates state behind that guard:
stopStreaming()on thefinal_answereventstopStreaming()on thecompleteeventstopStreaming()at normal end-of-streamcatchthat setserrorand clearsisStreamingisStreamingtherefore staystrueforever, and becausefinalAnsweris written by the samesetStatecall, the answer never reaches the UI either.Impact
setStatesits behind the same guard, so the spinner never clears.console.errorandtoast.errorare outside the guard, so an error toast appears while the spinner keeps spinning and no error state is rendered.Reproduce
npm run dev/search, configure Ask models, ask a questionPOST /api/search/ask 200 in …)Fix
Set
mountedRef.current = trueon (re)mount, so the guards survive Strict Mode's remount.Regression test
frontend/src/lib/hooks/use-ask.test.tsxrendersuseAskinsideStrictMode(so the mount → unmount → remount sequence actually runs) and drives a mocked SSE stream through both terminal paths: a normalfinal_answer+complete, and an in-banderrorevent.The existing suite could not catch this because nothing exercised a remount.
The new test fails on the pre-fix hook. With
mountedRef.current = truereverted:Verification
npm run test:npx eslint src/lib/hooks/use-ask.ts src/lib/hooks/use-ask.test.tsx— no output (clean)npx tsc --noEmit— no output (clean)npm run build— succeeded, all routes compiled.