From 69cc3f8e1e46f1a0c4702371a9544e74601f4a04 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Fri, 6 Feb 2026 20:33:52 -0600 Subject: [PATCH] fix(web): Remove re-throw from loadConversation to prevent unhandled rejections - Make loadConversation fully self-contained like sendMessage (handle errors internally via state, onError callback, and structured logging) - Remove duplicate try/catch+log from Chat.tsx imperative handle - Replace re-throw tests with delegation and no-throw tests - Add hook-level loadConversation error path tests (getIdea rejection) Co-Authored-By: Claude Opus 4.6 --- apps/web/src/components/chat/Chat.test.tsx | 59 +++------------------- apps/web/src/components/chat/Chat.tsx | 7 +-- apps/web/src/hooks/useChat.test.ts | 38 ++++++++++++++ apps/web/src/hooks/useChat.ts | 1 - 4 files changed, 47 insertions(+), 58 deletions(-) diff --git a/apps/web/src/components/chat/Chat.test.tsx b/apps/web/src/components/chat/Chat.test.tsx index 37ad5e1..8004250 100644 --- a/apps/web/src/components/chat/Chat.test.tsx +++ b/apps/web/src/components/chat/Chat.test.tsx @@ -103,7 +103,7 @@ describe("Chat", () => { }); describe("loadConversation via ref", () => { - it("should successfully load a conversation", async () => { + it("should delegate to useChat.loadConversation", async () => { const mockLoadConversation = vi.fn().mockResolvedValue(undefined); mockUseChat.mockReturnValue( createMockUseChatReturn({ loadConversation: mockLoadConversation }) @@ -117,9 +117,10 @@ describe("Chat", () => { expect(mockLoadConversation).toHaveBeenCalledWith("conv-123"); }); - it("should log error context and re-throw on network failure", async () => { - const networkError = new Error("Network request failed"); - const mockLoadConversation = vi.fn().mockRejectedValue(networkError); + it("should not re-throw when useChat.loadConversation handles errors internally", async () => { + // useChat.loadConversation handles errors internally (sets error state, logs, calls onError) + // and does NOT re-throw, so the imperative handle should resolve cleanly + const mockLoadConversation = vi.fn().mockResolvedValue(undefined); mockUseChat.mockReturnValue( createMockUseChatReturn({ loadConversation: mockLoadConversation }) ); @@ -127,57 +128,13 @@ describe("Chat", () => { const ref = createRef(); render(); - await expect(ref.current?.loadConversation("conv-123")).rejects.toThrow( - "Network request failed" - ); - - expect(consoleSpy).toHaveBeenCalledWith( - "Failed to load conversation", - expect.objectContaining({ - error: networkError, - conversationId: "conv-123", - }) - ); - }); - - it("should log error context and re-throw on API error (500)", async () => { - const apiError = new Error("Internal Server Error"); - const mockLoadConversation = vi.fn().mockRejectedValue(apiError); - mockUseChat.mockReturnValue( - createMockUseChatReturn({ loadConversation: mockLoadConversation }) - ); - - const ref = createRef(); - render(); - - await expect(ref.current?.loadConversation("conv-456")).rejects.toThrow( - "Internal Server Error" - ); - - expect(consoleSpy).toHaveBeenCalledWith( - "Failed to load conversation", - expect.objectContaining({ - conversationId: "conv-456", - }) - ); - }); - - it("should re-throw error for caller to handle", async () => { - const mockLoadConversation = vi.fn().mockRejectedValue(new Error("Auth failed")); - mockUseChat.mockReturnValue( - createMockUseChatReturn({ loadConversation: mockLoadConversation }) - ); - - const ref = createRef(); - render(); - - // Verify the error propagates to the caller - await expect(ref.current?.loadConversation("conv-789")).rejects.toThrow("Auth failed"); + // Should resolve without throwing + await expect(ref.current?.loadConversation("conv-123")).resolves.toBeUndefined(); }); }); describe("sendMessage delegation", () => { - it("should delegate to useChat.sendMessage without extra error handling", async () => { + it("should delegate to useChat.sendMessage", async () => { const mockSendMessage = vi.fn().mockResolvedValue(undefined); mockUseChat.mockReturnValue(createMockUseChatReturn({ sendMessage: mockSendMessage })); diff --git a/apps/web/src/components/chat/Chat.tsx b/apps/web/src/components/chat/Chat.tsx index b12f4a6..1cf0c6e 100644 --- a/apps/web/src/components/chat/Chat.tsx +++ b/apps/web/src/components/chat/Chat.tsx @@ -94,12 +94,7 @@ export const Chat = forwardRef(function Chat( // Expose methods to parent via ref useImperativeHandle(ref, () => ({ loadConversation: async (cId: string): Promise => { - try { - await loadConversation(cId); - } catch (err) { - console.error("Failed to load conversation", { error: err, conversationId: cId }); - throw err; - } + await loadConversation(cId); }, startNewConversation: (projectId?: string | null): void => { startNewConversation(projectId); diff --git a/apps/web/src/hooks/useChat.test.ts b/apps/web/src/hooks/useChat.test.ts index ba40434..dd0c716 100644 --- a/apps/web/src/hooks/useChat.test.ts +++ b/apps/web/src/hooks/useChat.test.ts @@ -377,6 +377,44 @@ describe("useChat", () => { expect(result.current.messages).toHaveLength(1); expect(result.current.messages[0]?.id).toBe("welcome"); }); + + it("should set sanitized error and call onError when getIdea rejects", async () => { + const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + mockGetIdea.mockRejectedValueOnce(new Error("Not found")); + + const onError = vi.fn(); + const { result } = renderHook(() => useChat({ onError })); + + await act(async () => { + await result.current.loadConversation("conv-missing"); + }); + + expect(result.current.error).toBe("Unable to load conversation. Please try again."); + expect(onError).toHaveBeenCalledWith(expect.any(Error)); + expect(consoleSpy).toHaveBeenCalledWith( + "Failed to load conversation", + expect.objectContaining({ + errorType: "LOAD_ERROR", + ideaId: "conv-missing", + timestamp: expect.any(String) as string, + }) + ); + expect(result.current.isLoading).toBe(false); + }); + + it("should not re-throw when getIdea rejects", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + mockGetIdea.mockRejectedValueOnce(new Error("Server error")); + + const { result } = renderHook(() => useChat()); + + // Should resolve without throwing - errors are handled internally + await act(async () => { + await expect(result.current.loadConversation("conv-err")).resolves.toBeUndefined(); + }); + + expect(result.current.error).toBe("Unable to load conversation. Please try again."); + }); }); describe("startNewConversation", () => { diff --git a/apps/web/src/hooks/useChat.ts b/apps/web/src/hooks/useChat.ts index 3f36f0e..564b3d1 100644 --- a/apps/web/src/hooks/useChat.ts +++ b/apps/web/src/hooks/useChat.ts @@ -290,7 +290,6 @@ export function useChat(options: UseChatOptions = {}): UseChatReturn { ideaId, timestamp: new Date().toISOString(), }); - throw err; } finally { setIsLoading(false); }