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 <noreply@anthropic.com>
This commit is contained in:
@@ -176,22 +176,19 @@ describe("Chat", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("sendMessage error handling", () => {
|
describe("sendMessage delegation", () => {
|
||||||
it("should log error when sendMessage fails", async () => {
|
it("should delegate to useChat.sendMessage without extra error handling", async () => {
|
||||||
const sendError = new Error("Send failed");
|
const mockSendMessage = vi.fn().mockResolvedValue(undefined);
|
||||||
const mockSendMessage = vi.fn().mockRejectedValue(sendError);
|
|
||||||
mockUseChat.mockReturnValue(createMockUseChatReturn({ sendMessage: mockSendMessage }));
|
mockUseChat.mockReturnValue(createMockUseChatReturn({ sendMessage: mockSendMessage }));
|
||||||
|
|
||||||
const ref = createRef<ChatRef>();
|
const ref = createRef<ChatRef>();
|
||||||
const { getByTestId } = render(<Chat ref={ref} />);
|
const { getByTestId } = render(<Chat ref={ref} />);
|
||||||
|
|
||||||
// Click the send button (which calls handleSendMessage)
|
|
||||||
const sendButton = getByTestId("chat-input");
|
const sendButton = getByTestId("chat-input");
|
||||||
sendButton.click();
|
sendButton.click();
|
||||||
|
|
||||||
// Wait for async handling
|
|
||||||
await vi.waitFor(() => {
|
await vi.waitFor(() => {
|
||||||
expect(consoleSpy).toHaveBeenCalledWith("Error sending message:", sendError);
|
expect(mockSendMessage).toHaveBeenCalledWith("test message");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -73,9 +73,6 @@ export const Chat = forwardRef<ChatRef, ChatProps>(function Chat(
|
|||||||
} = useChat({
|
} = useChat({
|
||||||
model: "llama3.2",
|
model: "llama3.2",
|
||||||
...(initialProjectId !== undefined && { projectId: initialProjectId }),
|
...(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)
|
// Connect to WebSocket for real-time updates (when we have a user)
|
||||||
@@ -180,11 +177,7 @@ export const Chat = forwardRef<ChatRef, ChatProps>(function Chat(
|
|||||||
|
|
||||||
const handleSendMessage = useCallback(
|
const handleSendMessage = useCallback(
|
||||||
async (content: string) => {
|
async (content: string) => {
|
||||||
try {
|
await sendMessage(content);
|
||||||
await sendMessage(content);
|
|
||||||
} catch (err) {
|
|
||||||
console.error("Error sending message:", err);
|
|
||||||
}
|
|
||||||
},
|
},
|
||||||
[sendMessage]
|
[sendMessage]
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -33,6 +33,9 @@ const mockSendChatMessage = chatApi.sendChatMessage as MockedFunction<
|
|||||||
const mockCreateConversation = ideasApi.createConversation as MockedFunction<
|
const mockCreateConversation = ideasApi.createConversation as MockedFunction<
|
||||||
typeof ideasApi.createConversation
|
typeof ideasApi.createConversation
|
||||||
>;
|
>;
|
||||||
|
const mockUpdateConversation = ideasApi.updateConversation as MockedFunction<
|
||||||
|
typeof ideasApi.updateConversation
|
||||||
|
>;
|
||||||
const mockGetIdea = ideasApi.getIdea as MockedFunction<typeof ideasApi.getIdea>;
|
const mockGetIdea = ideasApi.getIdea as MockedFunction<typeof ideasApi.getIdea>;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -156,6 +159,7 @@ describe("useChat", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it("should handle API errors gracefully", async () => {
|
it("should handle API errors gracefully", async () => {
|
||||||
|
vi.spyOn(console, "error").mockImplementation(() => undefined);
|
||||||
mockSendChatMessage.mockRejectedValueOnce(new Error("API Error"));
|
mockSendChatMessage.mockRejectedValueOnce(new Error("API Error"));
|
||||||
|
|
||||||
const onError = vi.fn();
|
const onError = vi.fn();
|
||||||
@@ -165,11 +169,11 @@ describe("useChat", () => {
|
|||||||
await result.current.sendMessage("Hello");
|
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));
|
expect(onError).toHaveBeenCalledWith(expect.any(Error));
|
||||||
// Should have welcome + user + error message
|
// Should have welcome + user + error message
|
||||||
expect(result.current.messages).toHaveLength(3);
|
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", () => {
|
describe("clearError", () => {
|
||||||
it("should clear error state", async () => {
|
it("should clear error state", async () => {
|
||||||
|
vi.spyOn(console, "error").mockImplementation(() => undefined);
|
||||||
mockSendChatMessage.mockRejectedValueOnce(new Error("Test error"));
|
mockSendChatMessage.mockRejectedValueOnce(new Error("Test error"));
|
||||||
|
|
||||||
const { result } = renderHook(() => useChat());
|
const { result } = renderHook(() => useChat());
|
||||||
@@ -411,7 +416,7 @@ describe("useChat", () => {
|
|||||||
await result.current.sendMessage("Hello");
|
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(() => {
|
act(() => {
|
||||||
result.current.clearError();
|
result.current.clearError();
|
||||||
@@ -505,10 +510,10 @@ describe("useChat", () => {
|
|||||||
await result.current.sendMessage("Hello");
|
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
|
// Should have welcome + user + error message
|
||||||
expect(result.current.messages).toHaveLength(3);
|
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 () => {
|
it("should keep assistant message visible when save fails", async () => {
|
||||||
@@ -586,9 +591,71 @@ describe("useChat", () => {
|
|||||||
const saveError = result2.current.error;
|
const saveError = result2.current.error;
|
||||||
|
|
||||||
// They should be different
|
// 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(saveError).toContain("Message sent but failed to save");
|
||||||
expect(llmError).not.toEqual(saveError);
|
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.");
|
||||||
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -214,16 +214,18 @@ export function useChat(options: UseChatOptions = {}): UseChatReturn {
|
|||||||
} catch (saveErr) {
|
} catch (saveErr) {
|
||||||
const saveErrorMsg =
|
const saveErrorMsg =
|
||||||
saveErr instanceof Error ? saveErr.message : "Unknown persistence error";
|
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", {
|
console.error("Failed to save conversation", {
|
||||||
error: saveErr,
|
error: saveErr,
|
||||||
errorType: "PERSISTENCE_ERROR",
|
errorType: "PERSISTENCE_ERROR",
|
||||||
conversationId,
|
conversationId,
|
||||||
|
detail: saveErrorMsg,
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
const errorMsg = err instanceof Error ? err.message : "Failed to send message";
|
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));
|
onError?.(err instanceof Error ? err : new Error(errorMsg));
|
||||||
console.error("Failed to send chat message", {
|
console.error("Failed to send chat message", {
|
||||||
error: err,
|
error: err,
|
||||||
@@ -240,7 +242,7 @@ export function useChat(options: UseChatOptions = {}): UseChatReturn {
|
|||||||
const errorMessage: Message = {
|
const errorMessage: Message = {
|
||||||
id: `error-${String(Date.now())}`,
|
id: `error-${String(Date.now())}`,
|
||||||
role: "assistant",
|
role: "assistant",
|
||||||
content: `Error: ${errorMsg}`,
|
content: "Something went wrong. Please try again.",
|
||||||
createdAt: new Date().toISOString(),
|
createdAt: new Date().toISOString(),
|
||||||
};
|
};
|
||||||
setMessages((prev) => [...prev, errorMessage]);
|
setMessages((prev) => [...prev, errorMessage]);
|
||||||
@@ -280,8 +282,15 @@ export function useChat(options: UseChatOptions = {}): UseChatReturn {
|
|||||||
setConversationTitle(idea.title ?? null);
|
setConversationTitle(idea.title ?? null);
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
const errorMsg = err instanceof Error ? err.message : "Failed to load conversation";
|
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));
|
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 {
|
} finally {
|
||||||
setIsLoading(false);
|
setIsLoading(false);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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", () => {
|
describe("storage error handling", () => {
|
||||||
it("should call onStorageError when localStorage save fails", () => {
|
it("should call onStorageError when localStorage save fails", () => {
|
||||||
const onStorageError = vi.fn();
|
const onStorageError = vi.fn();
|
||||||
|
|||||||
@@ -83,7 +83,12 @@ function saveState(
|
|||||||
try {
|
try {
|
||||||
window.localStorage.setItem(STORAGE_KEY, JSON.stringify(state));
|
window.localStorage.setItem(STORAGE_KEY, JSON.stringify(state));
|
||||||
} catch (error) {
|
} 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);
|
console.warn("Failed to save chat overlay state to localStorage:", error);
|
||||||
|
|
||||||
if (onStorageError && !notifiedErrors.has(errorName)) {
|
if (onStorageError && !notifiedErrors.has(errorName)) {
|
||||||
@@ -119,7 +124,7 @@ export function useChatOverlay(options: UseChatOverlayOptions = {}): UseChatOver
|
|||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
const minimize = useCallback(() => {
|
const minimize = useCallback(() => {
|
||||||
setState((prev) => ({ ...prev, isMinimized: true }));
|
setState((prev) => (prev.isOpen ? { ...prev, isMinimized: true } : prev));
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
const expand = useCallback(() => {
|
const expand = useCallback(() => {
|
||||||
@@ -134,7 +139,7 @@ export function useChatOverlay(options: UseChatOverlayOptions = {}): UseChatOver
|
|||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
const toggleMinimize = useCallback(() => {
|
const toggleMinimize = useCallback(() => {
|
||||||
setState((prev) => ({ ...prev, isMinimized: !prev.isMinimized }));
|
setState((prev) => (prev.isOpen ? { ...prev, isMinimized: !prev.isMinimized } : prev));
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
return {
|
return {
|
||||||
|
|||||||
Reference in New Issue
Block a user