From 30c24a66f6e3c5d7fca1e287cf458d74a52cc481 Mon Sep 17 00:00:00 2001 From: Sebastien Bruel <93573440+sbruel@users.noreply.github.com> Date: Fri, 12 Sep 2025 23:59:15 +0900 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=9F=20fix:=20Auto-fetch=20agents=20to?= =?UTF-8?q?=20fill=20Viewport=20in=20Marketplace=20Scroll=20(#9591)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- client/src/components/Agents/AgentGrid.tsx | 9 +- client/src/components/Agents/Marketplace.tsx | 4 +- .../tests/AgentGrid.integration.spec.tsx | 474 +++++++++++++++++- client/src/hooks/useInfiniteScroll.ts | 116 +++-- 4 files changed, 547 insertions(+), 56 deletions(-) diff --git a/client/src/components/Agents/AgentGrid.tsx b/client/src/components/Agents/AgentGrid.tsx index 9f64428398..ab821eb87a 100644 --- a/client/src/components/Agents/AgentGrid.tsx +++ b/client/src/components/Agents/AgentGrid.tsx @@ -13,7 +13,7 @@ interface AgentGridProps { category: string; // Currently selected category searchQuery: string; // Current search query onSelectAgent: (agent: t.Agent) => void; // Callback when agent is selected - scrollElement?: HTMLElement | null; // Parent scroll container for infinite scroll + scrollElementRef?: React.RefObject; // Parent scroll container ref for infinite scroll } /** @@ -23,7 +23,7 @@ const AgentGrid: React.FC = ({ category, searchQuery, onSelectAgent, - scrollElement, + scrollElementRef, }) => { const localize = useLocalize(); @@ -87,7 +87,7 @@ const AgentGrid: React.FC = ({ // Set up infinite scroll const { setScrollElement } = useInfiniteScroll({ hasNextPage, - isFetchingNextPage, + isLoading: isFetching || isFetchingNextPage, fetchNextPage: () => { if (hasNextPage && !isFetching) { fetchNextPage(); @@ -99,10 +99,11 @@ const AgentGrid: React.FC = ({ // Connect the scroll element when it's provided useEffect(() => { + const scrollElement = scrollElementRef?.current; if (scrollElement) { setScrollElement(scrollElement); } - }, [scrollElement, setScrollElement]); + }, [scrollElementRef, setScrollElement]); /** * Get category display name from API data or use fallback diff --git a/client/src/components/Agents/Marketplace.tsx b/client/src/components/Agents/Marketplace.tsx index 83aa453da9..42d72179a6 100644 --- a/client/src/components/Agents/Marketplace.tsx +++ b/client/src/components/Agents/Marketplace.tsx @@ -427,7 +427,7 @@ const AgentMarketplace: React.FC = ({ className = '' }) = category={displayCategory} searchQuery={searchQuery} onSelectAgent={handleAgentSelect} - scrollElement={scrollContainerRef.current} + scrollElementRef={scrollContainerRef} /> @@ -507,7 +507,7 @@ const AgentMarketplace: React.FC = ({ className = '' }) = category={nextCategory} searchQuery={searchQuery} onSelectAgent={handleAgentSelect} - scrollElement={scrollContainerRef.current} + scrollElementRef={scrollContainerRef} /> )} diff --git a/client/src/components/Agents/tests/AgentGrid.integration.spec.tsx b/client/src/components/Agents/tests/AgentGrid.integration.spec.tsx index 3b1444ac45..cad18ea809 100644 --- a/client/src/components/Agents/tests/AgentGrid.integration.spec.tsx +++ b/client/src/components/Agents/tests/AgentGrid.integration.spec.tsx @@ -1,5 +1,6 @@ import React from 'react'; -import { render, screen, fireEvent } from '@testing-library/react'; +import { render, screen, fireEvent, waitFor, act } from '@testing-library/react'; + import '@testing-library/jest-dom'; import AgentGrid from '../AgentGrid'; import type t from 'librechat-data-provider'; @@ -81,6 +82,115 @@ import { useMarketplaceAgentsInfiniteQuery } from '~/data-provider/Agents'; const mockUseMarketplaceAgentsInfiniteQuery = jest.mocked(useMarketplaceAgentsInfiniteQuery); +// Helper to create mock API response +const createMockResponse = ( + agentIds: string[], + hasMore: boolean, + afterCursor?: string, +): t.AgentListResponse => ({ + object: 'list', + data: agentIds.map( + (id) => + ({ + id, + name: `Agent ${id}`, + description: `Description for ${id}`, + created_at: Date.now(), + model: 'gpt-4', + tools: [], + instructions: '', + avatar: null, + provider: 'openai', + model_parameters: { + temperature: 0.7, + top_p: 1, + frequency_penalty: 0, + presence_penalty: 0, + maxContextTokens: 2000, + max_context_tokens: 2000, + max_output_tokens: 2000, + }, + }) as t.Agent, + ), + first_id: agentIds[0] || '', + last_id: agentIds[agentIds.length - 1] || '', + has_more: hasMore, + after: afterCursor, +}); + +// Helper to setup mock viewport +const setupViewport = (scrollHeight: number, clientHeight: number) => { + const listeners: { [key: string]: EventListener[] } = {}; + return { + scrollHeight, + clientHeight, + scrollTop: 0, + addEventListener: jest.fn((event: string, listener: EventListener) => { + if (!listeners[event]) { + listeners[event] = []; + } + listeners[event].push(listener); + }), + removeEventListener: jest.fn((event: string, listener: EventListener) => { + if (listeners[event]) { + listeners[event] = listeners[event].filter((l) => l !== listener); + } + }), + dispatchEvent: jest.fn((event: Event) => { + const eventListeners = listeners[event.type]; + if (eventListeners) { + eventListeners.forEach((listener) => listener(event)); + } + return true; + }), + } as unknown as HTMLElement; +}; + +// Helper to create mock infinite query return value +const createMockInfiniteQuery = ( + pages: t.AgentListResponse[], + options?: { + isLoading?: boolean; + hasNextPage?: boolean; + fetchNextPage?: jest.Mock; + isFetchingNextPage?: boolean; + }, +) => + ({ + data: { + pages, + pageParams: pages.map((_, i) => (i === 0 ? undefined : `cursor-${i * 6}`)), + }, + isLoading: options?.isLoading ?? false, + error: null, + isFetching: false, + hasNextPage: options?.hasNextPage ?? pages[pages.length - 1]?.has_more ?? false, + isFetchingNextPage: options?.isFetchingNextPage ?? false, + fetchNextPage: options?.fetchNextPage ?? jest.fn(), + refetch: jest.fn(), + // Add missing required properties for UseInfiniteQueryResult + isError: false, + isLoadingError: false, + isRefetchError: false, + isSuccess: true, + status: 'success' as const, + dataUpdatedAt: Date.now(), + errorUpdateCount: 0, + errorUpdatedAt: 0, + failureCount: 0, + failureReason: null, + fetchStatus: 'idle' as const, + isFetched: true, + isFetchedAfterMount: true, + isInitialLoading: false, + isPaused: false, + isPlaceholderData: false, + isPending: false, + isRefetching: false, + isStale: false, + remove: jest.fn(), + }) as any; + describe('AgentGrid Integration with useGetMarketplaceAgentsQuery', () => { const mockOnSelectAgent = jest.fn(); @@ -343,6 +453,15 @@ describe('AgentGrid Integration with useGetMarketplaceAgentsQuery', () => { }); describe('Infinite Scroll Functionality', () => { + beforeEach(() => { + // Silence console.log in tests + jest.spyOn(console, 'log').mockImplementation(() => {}); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + it('should show loading indicator when fetching next page', () => { mockUseMarketplaceAgentsInfiniteQuery.mockReturnValue({ ...defaultMockQueryResult, @@ -396,5 +515,358 @@ describe('AgentGrid Integration with useGetMarketplaceAgentsQuery', () => { expect(screen.queryByText("You've reached the end of the results")).not.toBeInTheDocument(); }); + + describe('Auto-fetch to fill viewport', () => { + it('should NOT auto-fetch when viewport is filled (5 agents, has_more=false)', async () => { + const mockResponse = createMockResponse(['1', '2', '3', '4', '5'], false); + const fetchNextPage = jest.fn(); + + mockUseMarketplaceAgentsInfiniteQuery.mockReturnValue( + createMockInfiniteQuery([mockResponse], { fetchNextPage }), + ); + + const scrollElement = setupViewport(500, 1000); // Content smaller than viewport + const scrollElementRef = { current: scrollElement }; + const Wrapper = createWrapper(); + + render( + + + , + ); + + // Wait for initial render + await waitFor(() => { + expect(screen.getAllByRole('gridcell')).toHaveLength(5); + }); + + // Wait to ensure no auto-fetch happens + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 200)); + }); + + // fetchNextPage should NOT be called since has_more is false + expect(fetchNextPage).not.toHaveBeenCalled(); + }); + + it('should auto-fetch when viewport not filled (7 agents, big viewport)', async () => { + const firstPage = createMockResponse(['1', '2', '3', '4', '5', '6'], true, 'cursor-6'); + const secondPage = createMockResponse(['7'], false); + let currentPages = [firstPage]; + const fetchNextPage = jest.fn(); + + // Mock that updates pages when fetchNextPage is called + mockUseMarketplaceAgentsInfiniteQuery.mockImplementation(() => + createMockInfiniteQuery(currentPages, { + fetchNextPage: jest.fn().mockImplementation(() => { + fetchNextPage(); + currentPages = [firstPage, secondPage]; + return Promise.resolve(); + }), + hasNextPage: true, + }), + ); + + const scrollElement = setupViewport(400, 1200); // Large viewport (content < viewport) + const scrollElementRef = { current: scrollElement }; + const Wrapper = createWrapper(); + + const { rerender } = render( + + + , + ); + + // Wait for initial 6 agents + await waitFor(() => { + expect(screen.getAllByRole('gridcell')).toHaveLength(6); + }); + + // Wait for ResizeObserver and auto-fetch to trigger + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 150)); + }); + + // Auto-fetch should have been triggered (multiple times due to reliability checks) + expect(fetchNextPage).toHaveBeenCalled(); + expect(fetchNextPage.mock.calls.length).toBeGreaterThanOrEqual(1); + + // Update mock data and re-render + currentPages = [firstPage, secondPage]; + rerender( + + + , + ); + + // Should now show all 7 agents + await waitFor(() => { + expect(screen.getAllByRole('gridcell')).toHaveLength(7); + }); + }); + + it('should NOT auto-fetch when viewport is filled (7 agents, small viewport)', async () => { + const firstPage = createMockResponse(['1', '2', '3', '4', '5', '6'], true, 'cursor-6'); + const fetchNextPage = jest.fn(); + + mockUseMarketplaceAgentsInfiniteQuery.mockReturnValue( + createMockInfiniteQuery([firstPage], { fetchNextPage, hasNextPage: true }), + ); + + const scrollElement = setupViewport(1200, 600); // Small viewport, content fills it + const scrollElementRef = { current: scrollElement }; + const Wrapper = createWrapper(); + + render( + + + , + ); + + // Wait for initial 6 agents + await waitFor(() => { + expect(screen.getAllByRole('gridcell')).toHaveLength(6); + }); + + // Wait to ensure no auto-fetch happens + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 200)); + }); + + // Should NOT auto-fetch since viewport is filled + expect(fetchNextPage).not.toHaveBeenCalled(); + }); + + it('should auto-fetch once to fill viewport then stop (20 agents)', async () => { + const allPages = [ + createMockResponse(['1', '2', '3', '4', '5', '6'], true, 'cursor-6'), + createMockResponse(['7', '8', '9', '10', '11', '12'], true, 'cursor-12'), + createMockResponse(['13', '14', '15', '16', '17', '18'], true, 'cursor-18'), + createMockResponse(['19', '20'], false), + ]; + + let currentPages = [allPages[0]]; + let fetchCount = 0; + const fetchNextPage = jest.fn(); + + mockUseMarketplaceAgentsInfiniteQuery.mockImplementation(() => + createMockInfiniteQuery(currentPages, { + fetchNextPage: jest.fn().mockImplementation(() => { + fetchCount++; + fetchNextPage(); + if (currentPages.length < 2) { + currentPages = allPages.slice(0, 2); + } + return Promise.resolve(); + }), + hasNextPage: currentPages.length < 2, + }), + ); + + const scrollElement = setupViewport(600, 1000); // Viewport fits ~12 agents + const scrollElementRef = { current: scrollElement }; + const Wrapper = createWrapper(); + + const { rerender } = render( + + + , + ); + + // Wait for initial 6 agents + await waitFor(() => { + expect(screen.getAllByRole('gridcell')).toHaveLength(6); + }); + + // Should auto-fetch to fill viewport + await waitFor( + () => { + expect(fetchNextPage).toHaveBeenCalledTimes(1); + }, + { timeout: 500 }, + ); + + // Simulate viewport being filled after 12 agents + Object.defineProperty(scrollElement, 'scrollHeight', { + value: 1200, + writable: true, + configurable: true, + }); + + currentPages = allPages.slice(0, 2); + rerender( + + + , + ); + + // Should show 12 agents + await waitFor(() => { + expect(screen.getAllByRole('gridcell')).toHaveLength(12); + }); + + // Wait to ensure no additional auto-fetch + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 200)); + }); + + // Should only have fetched once (to fill viewport) + expect(fetchCount).toBe(1); + expect(fetchNextPage).toHaveBeenCalledTimes(1); + }); + + it('should auto-fetch when viewport resizes to be taller (window resize)', async () => { + const firstPage = createMockResponse(['1', '2', '3', '4', '5', '6'], true, 'cursor-6'); + const secondPage = createMockResponse(['7', '8', '9', '10', '11', '12'], true, 'cursor-12'); + let currentPages = [firstPage]; + const fetchNextPage = jest.fn(); + let resizeObserverCallback: ResizeObserverCallback | null = null; + + // Mock that updates pages when fetchNextPage is called + mockUseMarketplaceAgentsInfiniteQuery.mockImplementation(() => + createMockInfiniteQuery(currentPages, { + fetchNextPage: jest.fn().mockImplementation(() => { + fetchNextPage(); + if (currentPages.length === 1) { + currentPages = [firstPage, secondPage]; + } + return Promise.resolve(); + }), + hasNextPage: currentPages.length === 1, + }), + ); + + // Mock ResizeObserver to capture the callback + const ResizeObserverMock = jest.fn().mockImplementation((callback) => { + resizeObserverCallback = callback; + return { + observe: jest.fn(), + disconnect: jest.fn(), + unobserve: jest.fn(), + }; + }); + global.ResizeObserver = ResizeObserverMock as any; + + // Start with a small viewport that fits the content + const scrollElement = setupViewport(800, 600); + const scrollElementRef = { current: scrollElement }; + const Wrapper = createWrapper(); + + const { rerender } = render( + + + , + ); + + // Wait for initial 6 agents + await waitFor(() => { + expect(screen.getAllByRole('gridcell')).toHaveLength(6); + }); + + // Verify ResizeObserver was set up + expect(ResizeObserverMock).toHaveBeenCalled(); + expect(resizeObserverCallback).not.toBeNull(); + + // Initially no fetch should happen as viewport is filled + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 100)); + }); + expect(fetchNextPage).not.toHaveBeenCalled(); + + // Simulate window resize - make viewport taller + Object.defineProperty(scrollElement, 'clientHeight', { + value: 1200, // Now taller than content + writable: true, + configurable: true, + }); + + // Trigger ResizeObserver callback to simulate resize detection + act(() => { + if (resizeObserverCallback) { + resizeObserverCallback( + [ + { + target: scrollElement, + contentRect: { + x: 0, + y: 0, + width: 800, + height: 1200, + top: 0, + right: 800, + bottom: 1200, + left: 0, + } as DOMRectReadOnly, + borderBoxSize: [], + contentBoxSize: [], + devicePixelContentBoxSize: [], + } as ResizeObserverEntry, + ], + {} as ResizeObserver, + ); + } + }); + + // Should trigger auto-fetch due to viewport now being larger than content + await waitFor( + () => { + expect(fetchNextPage).toHaveBeenCalledTimes(1); + }, + { timeout: 500 }, + ); + + // Update the component with new data + rerender( + + + , + ); + + // Should now show 12 agents after fetching + await waitFor(() => { + expect(screen.getAllByRole('gridcell')).toHaveLength(12); + }); + }); + }); }); }); diff --git a/client/src/hooks/useInfiniteScroll.ts b/client/src/hooks/useInfiniteScroll.ts index 4c1ac8dae9..9c35d6d183 100644 --- a/client/src/hooks/useInfiniteScroll.ts +++ b/client/src/hooks/useInfiniteScroll.ts @@ -1,9 +1,9 @@ -import { useCallback, useEffect, useRef } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { throttle } from 'lodash'; interface UseInfiniteScrollOptions { hasNextPage?: boolean; - isFetchingNextPage?: boolean; + isLoading?: boolean; fetchNextPage: () => void; threshold?: number; // Percentage of scroll position to trigger fetch (0-1) throttleMs?: number; // Throttle delay in milliseconds @@ -15,77 +15,95 @@ interface UseInfiniteScrollOptions { */ export const useInfiniteScroll = ({ hasNextPage = false, - isFetchingNextPage = false, + isLoading = false, fetchNextPage, threshold = 0.8, // Trigger when 80% scrolled throttleMs = 200, }: UseInfiniteScrollOptions) => { - const scrollElementRef = useRef(null); + // Monitor resizing of the scroll container + const resizeObserverRef = useRef(null); + const [scrollElement, setScrollElementState] = useState(null); - // Throttled scroll handler to prevent excessive API calls - const handleScroll = useCallback( - throttle(() => { - const element = scrollElementRef.current; - if (!element) return; + // Handler to check if we need to fetch more data + const handleNeedToFetch = useCallback(() => { + if (!scrollElement) return; - const { scrollTop, scrollHeight, clientHeight } = element; + const { scrollTop, scrollHeight, clientHeight } = scrollElement; - // Calculate scroll position as percentage - const scrollPosition = (scrollTop + clientHeight) / scrollHeight; + // Calculate scroll position as percentage + const scrollPosition = (scrollTop + clientHeight) / scrollHeight; - // Check if we've scrolled past the threshold and conditions are met - const shouldFetch = scrollPosition >= threshold && hasNextPage && !isFetchingNextPage; + // Check if we've scrolled past the threshold and conditions are met + const shouldFetch = scrollPosition >= threshold && hasNextPage && !isLoading; - if (shouldFetch) { - fetchNextPage(); - } - }, throttleMs), - [hasNextPage, isFetchingNextPage, fetchNextPage, threshold, throttleMs], + if (shouldFetch) { + fetchNextPage(); + } + }, [scrollElement, hasNextPage, isLoading, fetchNextPage, threshold]); + + // Create a throttled version - using useMemo to ensure it's created synchronously + const throttledHandleNeedToFetch = useMemo( + () => throttle(handleNeedToFetch, throttleMs), + [handleNeedToFetch, throttleMs], ); - // Set up scroll listener + // Clean up throttled function on unmount useEffect(() => { - const element = scrollElementRef.current; + return () => { + throttledHandleNeedToFetch.cancel?.(); + }; + }, [throttledHandleNeedToFetch]); + + // Check if we need to fetch more data when loading state changes (useful to fill content on first load) + useEffect(() => { + if (isLoading === false && scrollElement) { + // Use requestAnimationFrame to ensure DOM is ready after loading completes + const rafId = requestAnimationFrame(() => { + throttledHandleNeedToFetch(); + }); + return () => cancelAnimationFrame(rafId); + } + }, [isLoading, scrollElement, throttledHandleNeedToFetch]); + + // Set up scroll listener and ResizeObserver + useEffect(() => { + const element = scrollElement; if (!element) return; - // Remove any existing listener first - element.removeEventListener('scroll', handleScroll); + // Add the scroll listener + element.addEventListener('scroll', throttledHandleNeedToFetch, { passive: true }); - // Add the new listener - element.addEventListener('scroll', handleScroll, { passive: true }); + // Set up ResizeObserver to detect size changes + if (resizeObserverRef.current) { + resizeObserverRef.current.disconnect(); + } + + resizeObserverRef.current = new ResizeObserver(() => { + // Check if we need to fetch more data when container resizes + throttledHandleNeedToFetch(); + }); + + resizeObserverRef.current.observe(element); + + // Check immediately when element changes + throttledHandleNeedToFetch(); return () => { - element.removeEventListener('scroll', handleScroll); - // Clean up throttled function - handleScroll.cancel?.(); + element.removeEventListener('scroll', throttledHandleNeedToFetch); + // Clean up ResizeObserver + if (resizeObserverRef.current) { + resizeObserverRef.current.disconnect(); + resizeObserverRef.current = null; + } }; - }, [handleScroll]); - - // Additional effect to re-setup listeners when scroll element changes - useEffect(() => { - const element = scrollElementRef.current; - if (!element) return; - // Remove any existing listener first - element.removeEventListener('scroll', handleScroll); - - // Add the new listener - element.addEventListener('scroll', handleScroll, { passive: true }); - return () => { - element.removeEventListener('scroll', handleScroll); - // Clean up throttled function - handleScroll.cancel?.(); - }; - }, [scrollElementRef.current, handleScroll]); + }, [scrollElement, throttledHandleNeedToFetch]); // Function to manually set the scroll container const setScrollElement = useCallback((element: HTMLElement | null) => { - scrollElementRef.current = element; + setScrollElementState(element); }, []); return { setScrollElement, - scrollElementRef, }; }; - -export default useInfiniteScroll;