From ad2c63ac143675eb777d48e70827c7980e2f0129 Mon Sep 17 00:00:00 2001 From: israel Date: Sat, 28 Feb 2026 07:41:42 +0100 Subject: [PATCH] fix(platform): make failed status unconditionally terminal and remove dead code The SDK maps stream abort to 'failed' (not 'aborted') at the UIMessage level, which means several checks were dead code. More critically, when generation failed mid-tool-call, isUnfinishedToolTurn kept isLoading stuck forever because 'failed' was only terminal when the last part wasn't a tool part. - Make 'failed' bypass tool-turn check (fixes stuck loading on failure) - Remove dead 'aborted' status checks (SDK never surfaces this value) - Remove dead finishReason === 'cancelled' check (not a valid value) - Remove dead 'history.toast.titleEmpty' i18n key - Fix min-h mismatch between edit input and display span in sidebar --- .../chat/components/chat-history-sidebar.tsx | 2 +- .../__tests__/use-chat-loading-state.test.ts | 173 +++++------------- .../chat/hooks/use-chat-loading-state.ts | 39 +--- .../chat/hooks/use-message-processing.ts | 6 +- .../convex/lib/agent_chat/internal_actions.ts | 5 - services/platform/messages/en.json | 1 - 6 files changed, 59 insertions(+), 167 deletions(-) diff --git a/services/platform/app/features/chat/components/chat-history-sidebar.tsx b/services/platform/app/features/chat/components/chat-history-sidebar.tsx index 4d062a873..47184d5b5 100644 --- a/services/platform/app/features/chat/components/chat-history-sidebar.tsx +++ b/services/platform/app/features/chat/components/chat-history-sidebar.tsx @@ -242,7 +242,7 @@ export function ChatHistorySidebar({ }} className="absolute inset-0 cursor-pointer rounded-md" /> - + {chat.title}
diff --git a/services/platform/app/features/chat/hooks/__tests__/use-chat-loading-state.test.ts b/services/platform/app/features/chat/hooks/__tests__/use-chat-loading-state.test.ts index 04bd44d6e..a99c29559 100644 --- a/services/platform/app/features/chat/hooks/__tests__/use-chat-loading-state.test.ts +++ b/services/platform/app/features/chat/hooks/__tests__/use-chat-loading-state.test.ts @@ -122,7 +122,7 @@ describe('useChatLoadingState', () => { expect(result.current.isLoading).toBe(false); }); - it('returns true when last part is a tool part (unfinished tool turn)', () => { + it('returns false when failed mid-tool-call (failed is unconditionally terminal)', () => { const { result } = renderHook(() => useChatLoadingState({ isPending: false, @@ -132,130 +132,7 @@ describe('useChatLoadingState', () => { id: 'msg-1', order: 0, role: 'assistant', - status: 'success', - text: 'Let me create that for you.', - parts: [ - { type: 'text', text: 'Let me create that for you.' }, - { type: 'step-start' }, - { - type: 'tool-excel', - toolCallId: 'call-1', - input: { operation: 'generate' }, - state: 'input-available', - }, - ], - }), - ], - threadId: THREAD_A, - pendingThreadId: null, - }), - ); - - expect(result.current.isLoading).toBe(true); - }); - - it('returns false when tool parts exist but text part follows (completed tool turn)', () => { - const { result } = renderHook(() => - useChatLoadingState({ - isPending: false, - setIsPending, - uiMessages: [ - createUIMessage({ - id: 'msg-1', - order: 0, - role: 'assistant', - status: 'success', - text: 'Here are the results...', - parts: [ - { type: 'step-start' }, - { - type: 'tool-rag_search', - toolCallId: 'call-1', - input: { query: 'test' }, - output: { results: [] }, - state: 'output-available', - }, - { type: 'text', text: 'Here are the results...' }, - ], - }), - ], - threadId: THREAD_A, - pendingThreadId: null, - }), - ); - - expect(result.current.isLoading).toBe(false); - }); - - it('returns true when tool message has text preamble before tool call', () => { - const { result } = renderHook(() => - useChatLoadingState({ - isPending: false, - setIsPending, - uiMessages: [ - createUIMessage({ - id: 'msg-1', - order: 0, - role: 'assistant', - status: 'success', - text: 'I will create an Excel file for you.', - parts: [ - { - type: 'text', - text: 'I will create an Excel file for you.', - }, - { type: 'step-start' }, - { - type: 'tool-excel', - toolCallId: 'call-1', - input: { operation: 'generate' }, - state: 'input-available', - }, - ], - }), - ], - threadId: THREAD_A, - pendingThreadId: null, - }), - ); - - expect(result.current.isLoading).toBe(true); - }); - - it('returns false when last assistant message is aborted', () => { - const { result } = renderHook(() => - useChatLoadingState({ - isPending: false, - setIsPending, - uiMessages: [ - createUIMessage({ - id: 'msg-1', - order: 0, - role: 'assistant', - // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- runtime value exists but SDK types lack it - status: 'aborted' as UIMessage['status'], - }), - ], - threadId: THREAD_A, - pendingThreadId: null, - }), - ); - - expect(result.current.isLoading).toBe(false); - }); - - it('returns false when aborted mid-tool-call (aborted overrides unfinished tool turn)', () => { - const { result } = renderHook(() => - useChatLoadingState({ - isPending: false, - setIsPending, - uiMessages: [ - createUIMessage({ - id: 'msg-1', - order: 0, - role: 'assistant', - // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- runtime value exists but SDK types lack it - status: 'aborted' as UIMessage['status'], + status: 'failed', text: 'Let me look that up.', parts: [ { type: 'text', text: 'Let me look that up.' }, @@ -456,6 +333,52 @@ describe('useChatLoadingState', () => { expect(setIsPending).toHaveBeenCalledWith(false); }); + it('clears isPending when failed mid-tool-call', () => { + const userMsg = createUIMessage({ + id: 'msg-1', + order: 0, + role: 'user', + text: 'Hello', + }); + const failedToolMsg = createUIMessage({ + id: 'msg-2', + order: 1, + role: 'assistant', + text: 'Let me create that for you.', + status: 'failed', + parts: [ + { type: 'text', text: 'Let me create that for you.' }, + { type: 'step-start' }, + { + type: 'tool-excel', + toolCallId: 'call-1', + input: { operation: 'generate' }, + state: 'input-available', + }, + ], + }); + + const { rerender } = renderHook((props) => useChatLoadingState(props), { + initialProps: { + isPending: true, + setIsPending, + uiMessages: [userMsg] as UIMessage[] | undefined, + threadId: THREAD_A as string | undefined, + pendingThreadId: THREAD_A as string | null, + }, + }); + + rerender({ + isPending: true, + setIsPending, + uiMessages: [userMsg, failedToolMsg], + threadId: THREAD_A, + pendingThreadId: THREAD_A, + }); + + expect(setIsPending).toHaveBeenCalledWith(false); + }); + it('does not clear isPending while assistant is still streaming', () => { const userMsg = createUIMessage({ id: 'msg-1', diff --git a/services/platform/app/features/chat/hooks/use-chat-loading-state.ts b/services/platform/app/features/chat/hooks/use-chat-loading-state.ts index 933ef612e..4c536ed9c 100644 --- a/services/platform/app/features/chat/hooks/use-chat-loading-state.ts +++ b/services/platform/app/features/chat/hooks/use-chat-loading-state.ts @@ -10,35 +10,18 @@ interface UseChatLoadingStateParams { pendingThreadId: string | null; } -/** - * Checks whether the assistant message represents an in-progress tool turn - * (the final text response has not yet arrived). - * - * The SDK appends parts in message order: tool-call parts first, then - * tool-result merges into existing parts, then final text parts last. - * If the last meaningful part is a tool-* part, the final response - * hasn't arrived yet. - */ -function isUnfinishedToolTurn(message: UIMessage) { - const parts = message.parts; - if (!parts?.length) return false; - - for (let i = parts.length - 1; i >= 0; i--) { - const type = parts[i].type; - if (type === 'step-start' || type.startsWith('source-')) continue; - return type.startsWith('tool-'); - } - return false; -} - /** * Derives a single `isLoading` boolean that answers: "Is the AI turn active?" * - * The AI turn is considered complete (not loading) only when ALL three - * conditions are met: + * The AI turn is considered complete (not loading) only when BOTH conditions + * are met: * 1. The last message is from the assistant - * 2. The last meaningful part is not a tool part (final text has arrived) - * 3. The last message has a terminal status (success/failed) + * 2. The last message has a terminal status (success/failed) + * + * `failed` is unconditionally terminal — even mid-tool-call — because the + * SDK maps stream abort to `failed` (not `aborted`), and a failed generation + * cannot resume. Without this, a failure during a tool turn would leave + * isLoading stuck forever. * * When no messages exist, falls back to `isPending` to bridge the gap * between send and first subscription data. @@ -61,12 +44,8 @@ export function useChatLoadingState({ const status: string | undefined = lastMessage.status; if (lastMessage.role !== 'assistant') return true; - if (status === 'aborted') return false; - return !( - !isUnfinishedToolTurn(lastMessage) && - (status === 'success' || status === 'failed') - ); + return !(status === 'success' || status === 'failed'); }, [isPending, uiMessages]); useEffect(() => { diff --git a/services/platform/app/features/chat/hooks/use-message-processing.ts b/services/platform/app/features/chat/hooks/use-message-processing.ts index a3e98c647..05edd7a1f 100644 --- a/services/platform/app/features/chat/hooks/use-message-processing.ts +++ b/services/platform/app/features/chat/hooks/use-message-processing.ts @@ -201,11 +201,7 @@ export function useMessageProcessing( if (m.status === 'streaming') { streamingKeysRef.current.add(m.key); isStreaming = true; - } else if ( - m.status === 'success' || - m.status === 'failed' || - m.status === 'aborted' - ) { + } else if (m.status === 'success' || m.status === 'failed') { streamingKeysRef.current.delete(m.key); } else { isStreaming = streamingKeysRef.current.has(m.key); diff --git a/services/platform/convex/lib/agent_chat/internal_actions.ts b/services/platform/convex/lib/agent_chat/internal_actions.ts index 7ef054877..ad559178a 100644 --- a/services/platform/convex/lib/agent_chat/internal_actions.ts +++ b/services/platform/convex/lib/agent_chat/internal_actions.ts @@ -273,11 +273,6 @@ export const runAgentGeneration = internalAction({ }, ); - // User cancelled — no validation or error handling needed - if (result.finishReason === 'cancelled') { - return result; - } - // Validate response — save a failed message so the client exits loading if (!result.text?.trim()) { try { diff --git a/services/platform/messages/en.json b/services/platform/messages/en.json index 87bb3a535..5c9f901c0 100644 --- a/services/platform/messages/en.json +++ b/services/platform/messages/en.json @@ -2221,7 +2221,6 @@ "loadMore": "Load more", "loadingMore": "Loading...", "toast": { - "titleEmpty": "Chat title cannot be empty", "renameFailed": "Failed to rename chat" } },