fix(web): Remove re-throw from loadConversation to prevent unhandled rejections
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
ci/woodpecker/pr/woodpecker Pipeline failed

- 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 <noreply@anthropic.com>
This commit is contained in:
Jason Woltje
2026-02-06 20:33:52 -06:00
parent f64ca3871d
commit 69cc3f8e1e
4 changed files with 47 additions and 58 deletions

View File

@@ -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<ChatRef>();
render(<Chat ref={ref} />);
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<ChatRef>();
render(<Chat ref={ref} />);
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<ChatRef>();
render(<Chat ref={ref} />);
// 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 }));

View File

@@ -94,12 +94,7 @@ export const Chat = forwardRef<ChatRef, ChatProps>(function Chat(
// Expose methods to parent via ref
useImperativeHandle(ref, () => ({
loadConversation: async (cId: string): Promise<void> => {
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);

View File

@@ -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", () => {

View File

@@ -290,7 +290,6 @@ export function useChat(options: UseChatOptions = {}): UseChatReturn {
ideaId,
timestamp: new Date().toISOString(),
});
throw err;
} finally {
setIsLoading(false);
}