fix(chat): abort streaming fetch on unmount / stopGenerating#115
fix(chat): abort streaming fetch on unmount / stopGenerating#115zachdive wants to merge 3 commits into
Conversation
Wire an AbortController into useCreativeChatMutation and useParametricChatMutation so the fetch + stream reader are cancelled when the driving view unmounts or stopGenerating fires. Previously a mid-stream unmount (error boundary, route change) left the mutation orphaned — useIsMutating stayed truthy and the UI appeared stuck loading forever. - Module-level Map<conversationId, AbortController> + exported abortActiveStream(conversationId) helper. - Signal threaded through fetch; AbortError short-circuits onError so cancellation is silent (no Sentry, no placeholder message). - useEffect unmount cleanup calls abortActiveStream per hook instance. - Views' stopGenerating aborts client-side before the existing cancelRequest realtime signal. Streaming reader itself is untouched — only cancellation plumbing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Greptile SummaryAdds
Confidence Score: 4/5Not safe to merge until isAbortError is fixed; the abort guard is silently broken in the browser. One P1 defect: isAbortError only checks src/services/messageService.ts — the isAbortError function at line 29. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["stopGenerating() / unmount"] --> B["abortActiveStream(conversationId)"]
B --> C["controller.abort()"]
B --> D["delete from activeStreamControllers"]
C --> E["fetch / reader.read() throws DOMException"]
E --> F{"isAbortError(error)?\nerror instanceof Error && name==='AbortError'"}
F -->|"DOMException → instanceof Error = false\n❌ BROKEN PATH"| G["re-throw as real error"]
G --> H["onError fires WITHOUT guard"]
H --> I["Sentry.captureException 🔔"]
H --> J["insertMessageAsync 'An error occurred' 💬"]
F -->|"After fix: instanceof DOMException check"| K["wrap & re-throw AbortError"]
K --> L["onError → isAbortError → early return ✅"]
L --> M["No Sentry, no error message ✅"]
Reviews (3): Last reviewed commit: "fix(chat): address greptile v2 — shadowi..." | Re-trigger Greptile |
- isAbortError: replace `as` cast with `instanceof Error` per team rule. - Drop `controller.signal.aborted` fallback in the abort detection so a real error that happens to coincide with an aborted signal is not silently dropped. AbortError.name alone is sufficient — both the fetch-side DOMException and our manually-thrown error carry it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop the inner `conversationId` parameter from both chat mutationFn signatures. It shadowed the outer hook binding used by the useEffect cleanup and would silently miss the registry entry if ever called with a mismatched id. All three call sites (send/retry/edit) already passed `conversation.id`, so removing the key is a safe tightening. - In the abort catch, always throw a tagged AbortError instead of returning the partial `finalMessage`. Returning partial triggered onSuccess → messageInsertedConversationUpdate, which wrote the partial as the conversation leaf into the sidebar cache — diverging from DB when the server honors the abort before persisting. onError already filters AbortError silently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| function isAbortError(error: unknown): boolean { | ||
| return error instanceof Error && error.name === 'AbortError'; | ||
| } |
There was a problem hiding this comment.
isAbortError will not match the DOMException thrown by browser fetch/reader abort
In browsers, fetch() (and reader.read()) abort by throwing a DOMException with name === 'AbortError' — not a regular Error. DOMException does not extend Error on the browser's prototype chain, so error instanceof Error evaluates to false for every real abort thrown by the Fetch API. isAbortError() then returns false, the catch block re-throws the DOMException as a live error, onError skips its guard, Sentry fires, and the "An error occurred" placeholder is inserted — the exact behaviours this PR was meant to suppress.
| function isAbortError(error: unknown): boolean { | |
| return error instanceof Error && error.name === 'AbortError'; | |
| } | |
| function isAbortError(error: unknown): boolean { | |
| return (error instanceof DOMException || error instanceof Error) && error.name === 'AbortError'; | |
| } |
Summary
AbortControllerintouseCreativeChatMutationanduseParametricChatMutationso the streamingfetch+ reader are cancelled when the view unmounts orstopGeneratingfires.pagehide) left the mutation orphaned —useIsMutating(['parametric-chat', conversationId])stayed truthy andisLoadingnever cleared.What changed
Map<conversationId, AbortController>registry + exportedabortActiveStream(id)so the View can abort externally.signal: controller.signalthreaded through bothfetchcalls; catch block tags abort asAbortErrorandonErrorshort-circuits — no Sentry noise, no "An error occurred" placeholder message written to the thread.useEffectunmount cleanup in each hook invokesabortActiveStream(conversationId)— this is the path that actually fixes the orphan case.stopGeneratinginParametricEditorViewandCreativeEditorViewnow aborts client-side before the existingcancelRequestrealtime signal (client drops connection → server-side signal tells the edge function to wind down).Test plan
useIsMutatingreturns 0.stopGeneratingbutton still works end-to-end for both parametric and creative chat.🤖 Generated with Claude Code
Summary by cubic
Abort streaming chat requests on unmount and when Stop Generating is pressed to prevent orphaned mutations and stuck loading states. Adds a per-conversation
AbortControllerand ensures aborts are silent and never commit partial messages.AbortControlleron unmount andstopGenerating; views callabortActiveStream(conversationId)beforecancelRequest.signalintofetch; aborts throwAbortErrorand are ignored byonError(no Sentry, no user error).conversationIdparam from mutation fns to avoid abort registry misses; callers no longer pass it.useIsMutating(['parametric-chat', conversationId])stayed truthy after mid-stream unmount.isAbortErrorusesinstanceof Error; droppedsignal.abortedfallback.Written for commit 375b5db. Summary will update on new commits.