fix(dashboard): address review comments on PR #1099
- fix(perf): use messagesRef.current in loadMoreMessages to avoid recreating callback on every streamed token; messagesRef is already kept in sync on every render so no useEffect needed — removes `messages` from useCallback deps - fix(api): reject invalid order query param with 400 instead of silently ignoring; valid values are 'asc' and 'desc' Tests: - useChat: loadMoreMessages identity is stable when messages array changes - chat-routes: GET /messages?order=invalid returns 400
This commit is contained in:
@@ -2141,6 +2141,52 @@ describe("useChat", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("loadMoreMessages callback is stable when messages array changes (no re-create on streaming)", async () => {
|
||||
const session = makeSession({ id: "session-001", agentId: "agent-001" });
|
||||
mockFetchChatSessions.mockResolvedValueOnce({ sessions: [session] });
|
||||
|
||||
// 50 messages so hasMoreMessages=true
|
||||
const make50 = () =>
|
||||
Array.from({ length: 50 }, (_, i) =>
|
||||
makeMessage({ id: `msg-${i}`, sessionId: "session-001", role: "user", content: `m${i}`, createdAt: `2026-04-08T00:00:${String(i).padStart(2, "0")}.000Z` })
|
||||
);
|
||||
mockFetchChatMessages.mockResolvedValueOnce({ messages: make50() });
|
||||
|
||||
// Minimal streaming mock — returns immediately so sendMessage won't hang
|
||||
mockStreamChatResponse.mockImplementation(() => ({ close: vi.fn(), isConnected: () => false }));
|
||||
|
||||
const { result } = renderHook(() => useChat());
|
||||
|
||||
await waitFor(() => {
|
||||
expect(result.current.sessions).toHaveLength(1);
|
||||
});
|
||||
|
||||
act(() => {
|
||||
result.current.selectSession("session-001");
|
||||
});
|
||||
|
||||
await waitFor(() => {
|
||||
expect(result.current.messages).toHaveLength(50);
|
||||
expect(result.current.hasMoreMessages).toBe(true);
|
||||
});
|
||||
|
||||
// Capture callback identity before messages change
|
||||
const loadMoreBefore = result.current.loadMoreMessages;
|
||||
|
||||
// sendMessage adds an optimistic user message → new messages array reference
|
||||
act(() => {
|
||||
void result.current.sendMessage("hello");
|
||||
});
|
||||
|
||||
await waitFor(() => {
|
||||
// Optimistic user message was appended
|
||||
expect(result.current.messages.length).toBeGreaterThan(50);
|
||||
});
|
||||
|
||||
// loadMoreMessages must NOT have been recreated despite messages array changing
|
||||
expect(result.current.loadMoreMessages).toBe(loadMoreBefore);
|
||||
});
|
||||
|
||||
it("filters sessions by search query", async () => {
|
||||
mockFetchChatSessions.mockResolvedValueOnce({
|
||||
sessions: [
|
||||
|
||||
@@ -742,13 +742,16 @@ export function useChat(
|
||||
);
|
||||
|
||||
// Load more messages (pagination — use before cursor for oldest displayed message)
|
||||
// messagesRef is assigned on every render; reading from the ref here avoids
|
||||
// closing over `messages` and prevents this callback from being recreated on
|
||||
// every streamed token (which would cause the IntersectionObserver to churn).
|
||||
const loadMoreMessages = useCallback(async () => {
|
||||
if (!activeSession || !hasMoreMessages) return;
|
||||
// messages[0] is the oldest visible message; fetch older ones using its createdAt as cursor
|
||||
const cursor = messages[0]?.createdAt;
|
||||
// messagesRef.current[0] is the oldest visible message; fetch older ones using its createdAt
|
||||
const cursor = messagesRef.current[0]?.createdAt;
|
||||
if (!cursor) return;
|
||||
await loadMessages(activeSession.id, { before: cursor });
|
||||
}, [activeSession, hasMoreMessages, loadMessages, messages]);
|
||||
}, [activeSession, hasMoreMessages, loadMessages]);
|
||||
|
||||
const stopStreaming = useCallback(() => {
|
||||
if (!activeSession) return;
|
||||
|
||||
@@ -936,6 +936,19 @@ describe("Chat API Routes", () => {
|
||||
order: "desc",
|
||||
}));
|
||||
});
|
||||
|
||||
it("returns 400 for invalid order value", async () => {
|
||||
mockGetSession.mockReturnValue(sampleSession);
|
||||
|
||||
const response = await request(
|
||||
app,
|
||||
"GET",
|
||||
"/api/chat/sessions/chat-abc123/messages?order=invalid",
|
||||
);
|
||||
|
||||
expect(response.status).toBe(400);
|
||||
expect((response.body as any).error).toMatch(/order/i);
|
||||
});
|
||||
});
|
||||
|
||||
describe("POST /api/chat/sessions/:id/cancel", () => {
|
||||
|
||||
@@ -352,6 +352,10 @@ export function registerChatRoutes(ctx: ApiRoutesContext, deps: ChatRouteDeps):
|
||||
throw badRequest("offset must be a non-negative integer");
|
||||
}
|
||||
|
||||
if (order !== undefined && order !== "asc" && order !== "desc") {
|
||||
throw badRequest('order must be "asc" or "desc"');
|
||||
}
|
||||
|
||||
const effectiveLimit = Math.min(limit, 200);
|
||||
|
||||
const messages = chatStore.getMessages(sessionId, {
|
||||
|
||||
Reference in New Issue
Block a user