mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site): prevent infinite scroll from spamming duplicate chat list requests (#23075)
## Problem The agents sidebar infinite scroll was spamming the `/api/v2/chats` endpoint with duplicate requests at the same offset, caused by the `LoadMoreSentinel` component. ### Root cause `onLoadMore` is an inline arrow function (`() => void chatsQuery.fetchNextPage()`), creating a **new function reference on every render**. The `useEffect` in `LoadMoreSentinel` depended on `[onLoadMore]`, so it tore down and re-created the `IntersectionObserver` on every render. Each new observer immediately fired its callback when the sentinel was already visible, triggering duplicate fetches. ## Fix - Store `onLoadMore` and `isFetchingNextPage` in **refs** so the observer callback always reads the latest values without needing to tear down/re-create. - Create the `IntersectionObserver` **once on mount** (empty deps array). - **Guard** against calling `onLoadMore` while `isFetchingNextPage` is true. ## Tests - **LoadMoreSentinel behavior tests** (6 tests): verifies no duplicate calls across re-renders, proper `isFetchingNextPage` gating, ref-based observer stability, and correct resume after fetch completes. - **`infiniteChats` query factory tests** (6 tests): covers `getNextPageParam` and `queryFn` offset computation to prevent pagination regressions.
This commit is contained in:
@@ -10,6 +10,7 @@ import {
|
||||
chatCostUsersKey,
|
||||
chatKey,
|
||||
chatsKey,
|
||||
infiniteChats,
|
||||
unarchiveChat,
|
||||
} from "./chats";
|
||||
|
||||
@@ -17,6 +18,7 @@ vi.mock("api/api", () => ({
|
||||
API: {
|
||||
archiveChat: vi.fn(),
|
||||
unarchiveChat: vi.fn(),
|
||||
getChats: vi.fn(),
|
||||
getChatCostSummary: vi.fn(),
|
||||
getChatCostUsers: vi.fn(),
|
||||
},
|
||||
@@ -328,3 +330,72 @@ describe("chat cost query factories", () => {
|
||||
expect(API.getChatCostUsers).toHaveBeenCalledWith(params);
|
||||
});
|
||||
});
|
||||
|
||||
describe("infiniteChats", () => {
|
||||
const PAGE_LIMIT = 50;
|
||||
|
||||
describe("getNextPageParam", () => {
|
||||
it("returns undefined when lastPage has fewer items than the limit", () => {
|
||||
const { getNextPageParam } = infiniteChats();
|
||||
const lastPage = Array.from({ length: PAGE_LIMIT - 1 }, (_, i) =>
|
||||
makeChat(`chat-${i}`),
|
||||
);
|
||||
expect(getNextPageParam(lastPage, [lastPage])).toBeUndefined();
|
||||
});
|
||||
|
||||
it("returns pages.length + 1 when lastPage has exactly the limit", () => {
|
||||
const { getNextPageParam } = infiniteChats();
|
||||
const lastPage = Array.from({ length: PAGE_LIMIT }, (_, i) =>
|
||||
makeChat(`chat-${i}`),
|
||||
);
|
||||
const pages = [lastPage];
|
||||
expect(getNextPageParam(lastPage, pages)).toBe(pages.length + 1);
|
||||
});
|
||||
});
|
||||
|
||||
describe("queryFn", () => {
|
||||
it("computes offset 0 for pageParam 0", async () => {
|
||||
vi.mocked(API.getChats).mockResolvedValue([]);
|
||||
const { queryFn } = infiniteChats();
|
||||
await queryFn({ pageParam: 0 });
|
||||
expect(API.getChats).toHaveBeenCalledWith({
|
||||
limit: PAGE_LIMIT,
|
||||
offset: 0,
|
||||
});
|
||||
});
|
||||
|
||||
it("computes offset 0 for pageParam <= 0", async () => {
|
||||
vi.mocked(API.getChats).mockResolvedValue([]);
|
||||
const { queryFn } = infiniteChats();
|
||||
await queryFn({ pageParam: -1 });
|
||||
expect(API.getChats).toHaveBeenCalledWith({
|
||||
limit: PAGE_LIMIT,
|
||||
offset: 0,
|
||||
});
|
||||
});
|
||||
|
||||
it("computes correct offset for subsequent pages", async () => {
|
||||
vi.mocked(API.getChats).mockResolvedValue([]);
|
||||
const { queryFn } = infiniteChats();
|
||||
|
||||
await queryFn({ pageParam: 2 });
|
||||
expect(API.getChats).toHaveBeenCalledWith({
|
||||
limit: PAGE_LIMIT,
|
||||
offset: PAGE_LIMIT,
|
||||
});
|
||||
|
||||
await queryFn({ pageParam: 3 });
|
||||
expect(API.getChats).toHaveBeenCalledWith({
|
||||
limit: PAGE_LIMIT,
|
||||
offset: PAGE_LIMIT * 2,
|
||||
});
|
||||
});
|
||||
|
||||
it("throws when pageParam is not a number", () => {
|
||||
const { queryFn } = infiniteChats();
|
||||
expect(() => queryFn({ pageParam: "bad" })).toThrow(
|
||||
"pageParam must be a number",
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,276 @@
|
||||
import {
|
||||
MockAppearanceConfig,
|
||||
MockBuildInfo,
|
||||
MockDefaultOrganization,
|
||||
MockEntitlements,
|
||||
MockUserOwner,
|
||||
} from "testHelpers/entities";
|
||||
import { act, render } from "@testing-library/react";
|
||||
import type * as TypesGen from "api/typesGenerated";
|
||||
import type { Chat } from "api/typesGenerated";
|
||||
import { ThemeOverride } from "contexts/ThemeProvider";
|
||||
import { DashboardContext } from "modules/dashboard/DashboardProvider";
|
||||
import type { FC, PropsWithChildren } from "react";
|
||||
import { QueryClient, QueryClientProvider } from "react-query";
|
||||
import { MemoryRouter } from "react-router";
|
||||
import themes, { DEFAULT_THEME } from "theme";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { AgentsSidebar } from "./AgentsSidebar";
|
||||
|
||||
// ---- IntersectionObserver mock ----
|
||||
|
||||
type IOCallback = (entries: Array<{ isIntersecting: boolean }>) => void;
|
||||
let observerCallback: IOCallback | null = null;
|
||||
let observeCount = 0;
|
||||
|
||||
class MockIntersectionObserver {
|
||||
observe = vi.fn(() => {
|
||||
observeCount++;
|
||||
});
|
||||
disconnect = vi.fn();
|
||||
unobserve = vi.fn();
|
||||
|
||||
constructor(cb: IOCallback) {
|
||||
observerCallback = cb;
|
||||
}
|
||||
}
|
||||
|
||||
// ---- Auth mock ----
|
||||
|
||||
vi.mock("hooks", async () => {
|
||||
const actual = await vi.importActual("hooks");
|
||||
return {
|
||||
...actual,
|
||||
useAuthenticated: () => ({
|
||||
user: MockUserOwner,
|
||||
permissions: {},
|
||||
signOut: vi.fn(),
|
||||
}),
|
||||
};
|
||||
});
|
||||
|
||||
// ---- Helpers ----
|
||||
|
||||
const oneWeekAgo = new Date(Date.now() - 7 * 24 * 60 * 60 * 1000).toISOString();
|
||||
|
||||
const buildChat = (overrides: Partial<Chat> = {}): Chat => ({
|
||||
id: "chat-default",
|
||||
owner_id: "owner-1",
|
||||
title: "Agent",
|
||||
status: "completed",
|
||||
last_model_config_id: "model-1",
|
||||
created_at: oneWeekAgo,
|
||||
updated_at: oneWeekAgo,
|
||||
archived: false,
|
||||
last_error: null,
|
||||
...overrides,
|
||||
});
|
||||
|
||||
const dashboardValue = {
|
||||
entitlements: MockEntitlements,
|
||||
experiments: [] as TypesGen.Experiment[],
|
||||
appearance: MockAppearanceConfig,
|
||||
buildInfo: MockBuildInfo,
|
||||
organizations: [MockDefaultOrganization],
|
||||
showOrganizations: false,
|
||||
canViewOrganizationSettings: false,
|
||||
};
|
||||
|
||||
const Wrapper: FC<PropsWithChildren> = ({ children }) => {
|
||||
const queryClient = new QueryClient({
|
||||
defaultOptions: {
|
||||
queries: { retry: false, refetchOnWindowFocus: false },
|
||||
},
|
||||
});
|
||||
return (
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<ThemeOverride theme={themes[DEFAULT_THEME]}>
|
||||
<MemoryRouter initialEntries={["/agents"]}>
|
||||
<DashboardContext.Provider value={dashboardValue}>
|
||||
{children}
|
||||
</DashboardContext.Provider>
|
||||
</MemoryRouter>
|
||||
</ThemeOverride>
|
||||
</QueryClientProvider>
|
||||
);
|
||||
};
|
||||
|
||||
const defaultProps: React.ComponentProps<typeof AgentsSidebar> = {
|
||||
chats: [buildChat({ id: "chat-1", title: "Chat One" })],
|
||||
chatErrorReasons: {},
|
||||
modelOptions: [],
|
||||
modelConfigs: [],
|
||||
onArchiveAgent: vi.fn(),
|
||||
onUnarchiveAgent: vi.fn(),
|
||||
onArchiveAndDeleteWorkspace: vi.fn(),
|
||||
onNewAgent: vi.fn(),
|
||||
isCreating: false,
|
||||
archivedFilter: "active" as const,
|
||||
};
|
||||
|
||||
// ---- Tests ----
|
||||
|
||||
describe("AgentsSidebar load-more behavior", () => {
|
||||
beforeEach(() => {
|
||||
observerCallback = null;
|
||||
observeCount = 0;
|
||||
vi.stubGlobal("IntersectionObserver", MockIntersectionObserver);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("calls onLoadMore when the sentinel becomes visible", () => {
|
||||
const onLoadMore = vi.fn();
|
||||
render(
|
||||
<Wrapper>
|
||||
<AgentsSidebar {...defaultProps} hasNextPage onLoadMore={onLoadMore} />
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
act(() => {
|
||||
observerCallback?.([{ isIntersecting: true }]);
|
||||
});
|
||||
|
||||
expect(onLoadMore).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("does NOT call onLoadMore when isFetchingNextPage is true", () => {
|
||||
const onLoadMore = vi.fn();
|
||||
render(
|
||||
<Wrapper>
|
||||
<AgentsSidebar
|
||||
{...defaultProps}
|
||||
hasNextPage
|
||||
onLoadMore={onLoadMore}
|
||||
isFetchingNextPage
|
||||
/>
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
act(() => {
|
||||
observerCallback?.([{ isIntersecting: true }]);
|
||||
});
|
||||
|
||||
expect(onLoadMore).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does NOT recreate the observer when re-rendered with a new onLoadMore reference", () => {
|
||||
const onLoadMore1 = vi.fn();
|
||||
const { rerender } = render(
|
||||
<Wrapper>
|
||||
<AgentsSidebar {...defaultProps} hasNextPage onLoadMore={onLoadMore1} />
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
const countAfterMount = observeCount;
|
||||
|
||||
// Re-render with a brand-new function reference, which was the
|
||||
// original bug trigger.
|
||||
const onLoadMore2 = vi.fn();
|
||||
rerender(
|
||||
<Wrapper>
|
||||
<AgentsSidebar {...defaultProps} hasNextPage onLoadMore={onLoadMore2} />
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
// The observer should NOT have been torn down and recreated.
|
||||
expect(observeCount).toBe(countAfterMount);
|
||||
|
||||
// The new callback should still be the one invoked.
|
||||
act(() => {
|
||||
observerCallback?.([{ isIntersecting: true }]);
|
||||
});
|
||||
expect(onLoadMore1).not.toHaveBeenCalled();
|
||||
expect(onLoadMore2).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("does NOT spam onLoadMore across multiple re-renders", () => {
|
||||
const onLoadMore = vi.fn();
|
||||
const { rerender } = render(
|
||||
<Wrapper>
|
||||
<AgentsSidebar {...defaultProps} hasNextPage onLoadMore={onLoadMore} />
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
// Sentinel becomes visible once.
|
||||
act(() => {
|
||||
observerCallback?.([{ isIntersecting: true }]);
|
||||
});
|
||||
expect(onLoadMore).toHaveBeenCalledTimes(1);
|
||||
|
||||
// Parent re-renders many times with new inline arrow callbacks
|
||||
// (the pattern that caused the original bug).
|
||||
for (let i = 0; i < 10; i++) {
|
||||
rerender(
|
||||
<Wrapper>
|
||||
<AgentsSidebar
|
||||
{...defaultProps}
|
||||
hasNextPage
|
||||
onLoadMore={() => onLoadMore()}
|
||||
/>
|
||||
</Wrapper>,
|
||||
);
|
||||
}
|
||||
|
||||
// Re-renders alone should NOT trigger additional onLoadMore calls;
|
||||
// only a new IntersectionObserver entry should.
|
||||
expect(onLoadMore).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("resumes loading after isFetchingNextPage goes from true to false", () => {
|
||||
const onLoadMore = vi.fn();
|
||||
const { rerender } = render(
|
||||
<Wrapper>
|
||||
<AgentsSidebar
|
||||
{...defaultProps}
|
||||
hasNextPage
|
||||
onLoadMore={onLoadMore}
|
||||
isFetchingNextPage
|
||||
/>
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
// Blocked while fetching.
|
||||
act(() => {
|
||||
observerCallback?.([{ isIntersecting: true }]);
|
||||
});
|
||||
expect(onLoadMore).not.toHaveBeenCalled();
|
||||
|
||||
// Fetch completes.
|
||||
rerender(
|
||||
<Wrapper>
|
||||
<AgentsSidebar
|
||||
{...defaultProps}
|
||||
hasNextPage
|
||||
onLoadMore={onLoadMore}
|
||||
isFetchingNextPage={false}
|
||||
/>
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
// Observer fires again while sentinel is still visible.
|
||||
act(() => {
|
||||
observerCallback?.([{ isIntersecting: true }]);
|
||||
});
|
||||
expect(onLoadMore).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("does NOT render the sentinel when hasNextPage is false", () => {
|
||||
const onLoadMore = vi.fn();
|
||||
render(
|
||||
<Wrapper>
|
||||
<AgentsSidebar
|
||||
{...defaultProps}
|
||||
hasNextPage={false}
|
||||
onLoadMore={onLoadMore}
|
||||
/>
|
||||
</Wrapper>,
|
||||
);
|
||||
|
||||
// No observer should have been created since the sentinel
|
||||
// is not rendered.
|
||||
expect(observeCount).toBe(0);
|
||||
});
|
||||
});
|
||||
@@ -903,22 +903,39 @@ const LoadMoreSentinel: FC<{
|
||||
isFetchingNextPage?: boolean;
|
||||
}> = ({ onLoadMore, isFetchingNextPage }) => {
|
||||
const sentinelRef = useRef<HTMLDivElement>(null);
|
||||
const onLoadMoreRef = useRef(onLoadMore);
|
||||
const isFetchingNextPageRef = useRef(isFetchingNextPage);
|
||||
|
||||
// Keep refs in sync with the latest prop values so the
|
||||
// observer callback always reads current state without
|
||||
// needing to tear down and re-create the observer.
|
||||
useEffect(() => {
|
||||
onLoadMoreRef.current = onLoadMore;
|
||||
}, [onLoadMore]);
|
||||
|
||||
useEffect(() => {
|
||||
isFetchingNextPageRef.current = isFetchingNextPage;
|
||||
}, [isFetchingNextPage]);
|
||||
|
||||
useEffect(() => {
|
||||
const el = sentinelRef.current;
|
||||
if (!el || !onLoadMore) return;
|
||||
if (!el) return;
|
||||
|
||||
const observer = new IntersectionObserver(
|
||||
(entries) => {
|
||||
if (entries[0]?.isIntersecting) {
|
||||
onLoadMore();
|
||||
if (
|
||||
entries[0]?.isIntersecting &&
|
||||
!isFetchingNextPageRef.current &&
|
||||
onLoadMoreRef.current
|
||||
) {
|
||||
onLoadMoreRef.current();
|
||||
}
|
||||
},
|
||||
{ threshold: 0 },
|
||||
);
|
||||
observer.observe(el);
|
||||
return () => observer.disconnect();
|
||||
}, [onLoadMore]);
|
||||
}, []);
|
||||
|
||||
return (
|
||||
<div ref={sentinelRef} className="flex items-center justify-center py-2">
|
||||
|
||||
Reference in New Issue
Block a user