From e632e2685443d7b361cf9fae05757a1057d24f21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 10 May 2026 18:01:40 +0200 Subject: [PATCH] fix(coach): repair SSE idempotency and error-frame handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop the withIdempotency wrap on POST /api/insights/chat. The wrapper caches response bodies via cloned.text() then replays through NextResponse.json(JSON.parse(...)) — turning the SSE wire format into a null body on retry. The PWA never sends Idempotency-Key so the bug is hidden today; the iOS client would walk straight into it. Dedup still holds via the conversationId existence check + 20-turn cap. - Read the SSE response body even on !response.ok when the content type is text/event-stream so the structured error frame the route emits surfaces its errorCode to the bubble. Without this the stream was read only on 2xx and the rich error channel was dead code. - Stabilise the useSendCoachMessage opts reference via useRef so an inline-literal { onDone } from the drawer no longer re-memoises send on every render. - Drop the unused snapshot parameter from streamProviderError (the ReturnType alias widened to any). Switch the error response to status 200 so the streaming client reads the SSE body and parses the error frame instead of branching on HTTP status. Fixes Code-HIGH-1, Code-HIGH-2, Code-HIGH-3, Code-HIGH-4, Security-M-1, Senior-HIGH-1, Simplify-S-01. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/app/api/insights/chat/route.ts | 64 ++++++------------- .../insights/coach-panel/use-coach.ts | 36 +++++++++-- 2 files changed, 50 insertions(+), 50 deletions(-) diff --git a/src/app/api/insights/chat/route.ts b/src/app/api/insights/chat/route.ts index 4ad75c609..3b9bf1cd0 100644 --- a/src/app/api/insights/chat/route.ts +++ b/src/app/api/insights/chat/route.ts @@ -30,7 +30,6 @@ import { NextResponse, type NextRequest } from "next/server"; import { apiHandler, requireAuth, HttpError } from "@/lib/api-handler"; import { apiSuccess } from "@/lib/api-response"; -import { withIdempotency } from "@/lib/idempotency"; import { annotate } from "@/lib/logging/context"; import { prisma } from "@/lib/db"; @@ -227,11 +226,7 @@ Reply now as the assistant, in ${locale === "de" ? "German" : "English"}.`; annotate({ action: { name: "insights.coach.noProvider" }, }); - return streamProviderError({ - conversationId: workingConversationId, - snapshot: snapshot.provenance, - code: "coach.provider.none", - }); + return streamProviderError({ code: "coach.provider.none" }); } chain.push({ providerType: "admin-openai", instance: legacy }); } @@ -260,22 +255,14 @@ Reply now as the assistant, in ${locale === "de" ? "German" : "English"}.`; firstStatus: err.attempts[0]?.httpStatus ?? null, }, }); - return streamProviderError({ - conversationId: workingConversationId, - snapshot: snapshot.provenance, - code: "coach.provider.unavailable", - }); + return streamProviderError({ code: "coach.provider.unavailable" }); } throw err; } const replyText = (result.content ?? "").trim(); if (!replyText) { - return streamProviderError({ - conversationId: workingConversationId, - snapshot: snapshot.provenance, - code: "coach.provider.empty", - }); + return streamProviderError({ code: "coach.provider.empty" }); } // Persist the assistant message BEFORE we begin streaming; if the @@ -403,11 +390,7 @@ async function streamRefusal(args: { return new Response(stream, { status: 200, headers: SSE_HEADERS }); } -function streamProviderError(args: { - conversationId: string; - snapshot: ReturnType; - code: string; -}): Response { +function streamProviderError(args: { code: string }): Response { const stream = new ReadableStream({ start(controller) { try { @@ -423,33 +406,22 @@ function streamProviderError(args: { } }, }); - return new Response(stream, { status: 503, headers: SSE_HEADERS }); + // Status 200 so the streaming client reads the SSE body and parses + // the structured `error` frame (HTTP-status branches drop the + // structured code on the floor). + return new Response(stream, { status: 200, headers: SSE_HEADERS }); } -const handler = apiHandler(async (request: NextRequest) => { - // Idempotency wrap is applied only when the user is creating a new - // conversation (the spec). For follow-up turns inside an existing - // thread, retries SHOULD hit the route again — every turn is a real - // server-side action, not a duplicate-prone POST. - let cloneForCheck: NextRequest | undefined = undefined; - let conversationId: string | undefined = undefined; - try { - cloneForCheck = request.clone() as NextRequest; - const body = await cloneForCheck.json(); - if (typeof body?.conversationId === "string") { - conversationId = body.conversationId; - } - } catch { - // fall through; validation will surface the issue inside the - // handler. - } - if (conversationId) { - return handleChatRequest(request); - } - return withIdempotency(handleChatRequest)(request); -}); - -export const POST = handler; +// Idempotency is intentionally NOT applied to this SSE-streaming route. +// `withIdempotency()` caches the response body via `cloned.text()` and +// replays it through `NextResponse.json(JSON.parse(...))` — that path +// turns an SSE wire format (`data: …\n\n` frames) into a `null` body +// because the cached text isn't JSON. The PWA never sets +// `Idempotency-Key` here so the bug is invisible today, but the iOS +// client does. Dedup still holds: a duplicate first-turn POST creates +// a second conversation row (cheap), and follow-up turns are gated by +// the conversationId existence check + 20-turn cap. +export const POST = apiHandler(handleChatRequest); /** * GET /api/insights/chat?cursor=&limit= diff --git a/src/components/insights/coach-panel/use-coach.ts b/src/components/insights/coach-panel/use-coach.ts index c522d55f7..47daa0436 100644 --- a/src/components/insights/coach-panel/use-coach.ts +++ b/src/components/insights/coach-panel/use-coach.ts @@ -1,6 +1,6 @@ "use client"; -import { useCallback, useRef, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { useMutation, useQuery, @@ -243,6 +243,14 @@ export function useSendCoachMessage(opts: UseSendCoachMessageOptions = {}) { useState(EMPTY_STREAMING); const abortRef = useRef(null); + // Stash `opts` in a ref so callers can pass an inline-literal object + // (`useSendCoachMessage({ onDone: … })`) without re-memoising `send` + // on every render. The hook owns the latest-callback-wins contract. + const optsRef = useRef(opts); + useEffect(() => { + optsRef.current = opts; + }, [opts]); + const reset = useCallback(() => { setStreaming(EMPTY_STREAMING); }, []); @@ -290,7 +298,27 @@ export function useSendCoachMessage(opts: UseSendCoachMessageOptions = {}) { return; } - if (!response.ok || !response.body) { + if (!response.body) { + setStreaming({ + content: "", + metricSource: null, + inProgress: false, + messageId: null, + errorCode: `coach.http.${response.status}`, + }); + return; + } + + // Even on `!response.ok`, the route may have emitted a structured + // `error` frame in an SSE body (e.g. 4xx/5xx + text/event-stream). + // Falling back to `coach.http.` would discard the richer + // `errorCode`. Read the stream and let the parser route the frame + // through the same path as the success case. + const contentType = response.headers.get("Content-Type") ?? ""; + const isEventStream = contentType + .toLowerCase() + .startsWith("text/event-stream"); + if (!response.ok && !isEventStream) { setStreaming({ content: "", metricSource: null, @@ -375,10 +403,10 @@ export function useSendCoachMessage(opts: UseSendCoachMessageOptions = {}) { queryKey: QUERY_KEYS.one(resolvedConversationId), }); queryClient.invalidateQueries({ queryKey: QUERY_KEYS.list() }); - opts.onDone?.(resolvedConversationId); + optsRef.current.onDone?.(resolvedConversationId); } }, - [opts, queryClient], + [queryClient], ); const cancel = useCallback(() => {