From f64ca3871d6c8f8a8eae49d7d4662275d39ee7ce Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Fri, 6 Feb 2026 20:25:03 -0600 Subject: [PATCH] fix(web): Address review findings for M4-LLM integration - Sanitize user-facing error messages (no raw API/DB errors) - Remove dead try/catch from Chat.tsx handleSendMessage - Add onError callback for persistence errors in useChat - Add console.error logging to loadConversation - Guard minimize/toggleMinimize against closed overlay state - Improve error dedup bucketing for non-DOMException errors - Add tests: non-Error throws, updateConversation failure, minimize/toggleMinimize guards Co-Authored-By: Claude Opus 4.6 --- apps/web/src/components/chat/Chat.test.tsx | 11 ++- apps/web/src/components/chat/Chat.tsx | 9 +-- apps/web/src/hooks/useChat.test.ts | 79 ++++++++++++++++++++-- apps/web/src/hooks/useChat.ts | 17 +++-- apps/web/src/hooks/useChatOverlay.test.ts | 31 +++++++++ apps/web/src/hooks/useChatOverlay.ts | 11 ++- 6 files changed, 130 insertions(+), 28 deletions(-) diff --git a/apps/web/src/components/chat/Chat.test.tsx b/apps/web/src/components/chat/Chat.test.tsx index 43205ac..37ad5e1 100644 --- a/apps/web/src/components/chat/Chat.test.tsx +++ b/apps/web/src/components/chat/Chat.test.tsx @@ -176,22 +176,19 @@ describe("Chat", () => { }); }); - describe("sendMessage error handling", () => { - it("should log error when sendMessage fails", async () => { - const sendError = new Error("Send failed"); - const mockSendMessage = vi.fn().mockRejectedValue(sendError); + describe("sendMessage delegation", () => { + it("should delegate to useChat.sendMessage without extra error handling", async () => { + const mockSendMessage = vi.fn().mockResolvedValue(undefined); mockUseChat.mockReturnValue(createMockUseChatReturn({ sendMessage: mockSendMessage })); const ref = createRef(); const { getByTestId } = render(); - // Click the send button (which calls handleSendMessage) const sendButton = getByTestId("chat-input"); sendButton.click(); - // Wait for async handling await vi.waitFor(() => { - expect(consoleSpy).toHaveBeenCalledWith("Error sending message:", sendError); + expect(mockSendMessage).toHaveBeenCalledWith("test message"); }); }); }); diff --git a/apps/web/src/components/chat/Chat.tsx b/apps/web/src/components/chat/Chat.tsx index c79dba7..b12f4a6 100644 --- a/apps/web/src/components/chat/Chat.tsx +++ b/apps/web/src/components/chat/Chat.tsx @@ -73,9 +73,6 @@ export const Chat = forwardRef(function Chat( } = useChat({ model: "llama3.2", ...(initialProjectId !== undefined && { projectId: initialProjectId }), - onError: (_err) => { - // Error is handled by the useChat hook's state - }, }); // Connect to WebSocket for real-time updates (when we have a user) @@ -180,11 +177,7 @@ export const Chat = forwardRef(function Chat( const handleSendMessage = useCallback( async (content: string) => { - try { - await sendMessage(content); - } catch (err) { - console.error("Error sending message:", err); - } + await sendMessage(content); }, [sendMessage] ); diff --git a/apps/web/src/hooks/useChat.test.ts b/apps/web/src/hooks/useChat.test.ts index 7c98325..ba40434 100644 --- a/apps/web/src/hooks/useChat.test.ts +++ b/apps/web/src/hooks/useChat.test.ts @@ -33,6 +33,9 @@ const mockSendChatMessage = chatApi.sendChatMessage as MockedFunction< const mockCreateConversation = ideasApi.createConversation as MockedFunction< typeof ideasApi.createConversation >; +const mockUpdateConversation = ideasApi.updateConversation as MockedFunction< + typeof ideasApi.updateConversation +>; const mockGetIdea = ideasApi.getIdea as MockedFunction; /** @@ -156,6 +159,7 @@ describe("useChat", () => { }); it("should handle API errors gracefully", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); mockSendChatMessage.mockRejectedValueOnce(new Error("API Error")); const onError = vi.fn(); @@ -165,11 +169,11 @@ describe("useChat", () => { await result.current.sendMessage("Hello"); }); - expect(result.current.error).toBe("API Error"); + expect(result.current.error).toBe("Unable to send message. Please try again."); expect(onError).toHaveBeenCalledWith(expect.any(Error)); // Should have welcome + user + error message expect(result.current.messages).toHaveLength(3); - expect(result.current.messages[2]?.content).toContain("Error: API Error"); + expect(result.current.messages[2]?.content).toBe("Something went wrong. Please try again."); }); }); @@ -403,6 +407,7 @@ describe("useChat", () => { describe("clearError", () => { it("should clear error state", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); mockSendChatMessage.mockRejectedValueOnce(new Error("Test error")); const { result } = renderHook(() => useChat()); @@ -411,7 +416,7 @@ describe("useChat", () => { await result.current.sendMessage("Hello"); }); - expect(result.current.error).toBe("Test error"); + expect(result.current.error).toBe("Unable to send message. Please try again."); act(() => { result.current.clearError(); @@ -505,10 +510,10 @@ describe("useChat", () => { await result.current.sendMessage("Hello"); }); - expect(result.current.error).toBe("Model not available"); + expect(result.current.error).toBe("Unable to send message. Please try again."); // Should have welcome + user + error message expect(result.current.messages).toHaveLength(3); - expect(result.current.messages[2]?.content).toContain("Error: Model not available"); + expect(result.current.messages[2]?.content).toBe("Something went wrong. Please try again."); }); it("should keep assistant message visible when save fails", async () => { @@ -586,9 +591,71 @@ describe("useChat", () => { const saveError = result2.current.error; // They should be different - expect(llmError).toBe("Timeout"); + expect(llmError).toBe("Unable to send message. Please try again."); expect(saveError).toContain("Message sent but failed to save"); expect(llmError).not.toEqual(saveError); }); + + it("should handle non-Error throws from LLM API", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + mockSendChatMessage.mockRejectedValueOnce("string error"); + + const onError = vi.fn(); + const { result } = renderHook(() => useChat({ onError })); + + await act(async () => { + await result.current.sendMessage("Hello"); + }); + + expect(result.current.error).toBe("Unable to send message. Please try again."); + expect(onError).toHaveBeenCalledWith(expect.any(Error)); + expect(result.current.messages[2]?.content).toBe("Something went wrong. Please try again."); + }); + + it("should handle non-Error throws from persistence layer", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + mockSendChatMessage.mockResolvedValueOnce(createMockChatResponse("OK")); + mockCreateConversation.mockRejectedValueOnce("DB string error"); + + const onError = vi.fn(); + const { result } = renderHook(() => useChat({ onError })); + + await act(async () => { + await result.current.sendMessage("Hello"); + }); + + // Assistant message should still be visible + expect(result.current.messages[2]?.content).toBe("OK"); + expect(result.current.error).toBe("Message sent but failed to save. Please try again."); + expect(onError).toHaveBeenCalledWith(expect.any(Error)); + }); + + it("should handle updateConversation failure for existing conversations", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + + // First message succeeds and creates conversation + mockSendChatMessage.mockResolvedValueOnce(createMockChatResponse("First response")); + mockCreateConversation.mockResolvedValueOnce(createMockIdea("conv-1", "Test", "")); + + const { result } = renderHook(() => useChat()); + + await act(async () => { + await result.current.sendMessage("First"); + }); + + expect(result.current.conversationId).toBe("conv-1"); + + // Second message succeeds but updateConversation fails + mockSendChatMessage.mockResolvedValueOnce(createMockChatResponse("Second response")); + mockUpdateConversation.mockRejectedValueOnce(new Error("Connection reset")); + + await act(async () => { + await result.current.sendMessage("Second"); + }); + + // Assistant message should still be visible + expect(result.current.messages[4]?.content).toBe("Second response"); + expect(result.current.error).toBe("Message sent but failed to save. Please try again."); + }); }); }); diff --git a/apps/web/src/hooks/useChat.ts b/apps/web/src/hooks/useChat.ts index 6a969f8..3f36f0e 100644 --- a/apps/web/src/hooks/useChat.ts +++ b/apps/web/src/hooks/useChat.ts @@ -214,16 +214,18 @@ export function useChat(options: UseChatOptions = {}): UseChatReturn { } catch (saveErr) { const saveErrorMsg = saveErr instanceof Error ? saveErr.message : "Unknown persistence error"; - setError(`Message sent but failed to save: ${saveErrorMsg}`); + setError("Message sent but failed to save. Please try again."); + onError?.(saveErr instanceof Error ? saveErr : new Error(saveErrorMsg)); console.error("Failed to save conversation", { error: saveErr, errorType: "PERSISTENCE_ERROR", conversationId, + detail: saveErrorMsg, }); } } catch (err) { const errorMsg = err instanceof Error ? err.message : "Failed to send message"; - setError(errorMsg); + setError("Unable to send message. Please try again."); onError?.(err instanceof Error ? err : new Error(errorMsg)); console.error("Failed to send chat message", { error: err, @@ -240,7 +242,7 @@ export function useChat(options: UseChatOptions = {}): UseChatReturn { const errorMessage: Message = { id: `error-${String(Date.now())}`, role: "assistant", - content: `Error: ${errorMsg}`, + content: "Something went wrong. Please try again.", createdAt: new Date().toISOString(), }; setMessages((prev) => [...prev, errorMessage]); @@ -280,8 +282,15 @@ export function useChat(options: UseChatOptions = {}): UseChatReturn { setConversationTitle(idea.title ?? null); } catch (err) { const errorMsg = err instanceof Error ? err.message : "Failed to load conversation"; - setError(errorMsg); + setError("Unable to load conversation. Please try again."); onError?.(err instanceof Error ? err : new Error(errorMsg)); + console.error("Failed to load conversation", { + error: err, + errorType: "LOAD_ERROR", + ideaId, + timestamp: new Date().toISOString(), + }); + throw err; } finally { setIsLoading(false); } diff --git a/apps/web/src/hooks/useChatOverlay.test.ts b/apps/web/src/hooks/useChatOverlay.test.ts index 5b2d37f..99eef23 100644 --- a/apps/web/src/hooks/useChatOverlay.test.ts +++ b/apps/web/src/hooks/useChatOverlay.test.ts @@ -369,6 +369,37 @@ describe("useChatOverlay", () => { }); }); + describe("minimize/toggleMinimize guards", () => { + it("should not allow minimize when overlay is closed", () => { + const { result } = renderHook(() => useChatOverlay()); + + // Overlay starts closed + expect(result.current.isOpen).toBe(false); + + act(() => { + result.current.minimize(); + }); + + // Should remain unchanged - cannot minimize when closed + expect(result.current.isOpen).toBe(false); + expect(result.current.isMinimized).toBe(false); + }); + + it("should not allow toggleMinimize when overlay is closed", () => { + const { result } = renderHook(() => useChatOverlay()); + + expect(result.current.isOpen).toBe(false); + + act(() => { + result.current.toggleMinimize(); + }); + + // Should remain unchanged - cannot toggle minimize when closed + expect(result.current.isOpen).toBe(false); + expect(result.current.isMinimized).toBe(false); + }); + }); + describe("storage error handling", () => { it("should call onStorageError when localStorage save fails", () => { const onStorageError = vi.fn(); diff --git a/apps/web/src/hooks/useChatOverlay.ts b/apps/web/src/hooks/useChatOverlay.ts index fefdf95..7e87cfd 100644 --- a/apps/web/src/hooks/useChatOverlay.ts +++ b/apps/web/src/hooks/useChatOverlay.ts @@ -83,7 +83,12 @@ function saveState( try { window.localStorage.setItem(STORAGE_KEY, JSON.stringify(state)); } catch (error) { - const errorName = error instanceof DOMException ? error.name : "UnknownError"; + const errorName = + error instanceof DOMException + ? error.name + : error instanceof Error + ? error.constructor.name + : "UnknownError"; console.warn("Failed to save chat overlay state to localStorage:", error); if (onStorageError && !notifiedErrors.has(errorName)) { @@ -119,7 +124,7 @@ export function useChatOverlay(options: UseChatOverlayOptions = {}): UseChatOver }, []); const minimize = useCallback(() => { - setState((prev) => ({ ...prev, isMinimized: true })); + setState((prev) => (prev.isOpen ? { ...prev, isMinimized: true } : prev)); }, []); const expand = useCallback(() => { @@ -134,7 +139,7 @@ export function useChatOverlay(options: UseChatOverlayOptions = {}): UseChatOver }, []); const toggleMinimize = useCallback(() => { - setState((prev) => ({ ...prev, isMinimized: !prev.isMinimized })); + setState((prev) => (prev.isOpen ? { ...prev, isMinimized: !prev.isMinimized } : prev)); }, []); return {