refactor: Simplify useInfiniteScroll hook API

- Remove redundant containerRef (useRef), keep only useState for container
- Rename return value from 'ref' to 'setRef' for clearer API
- Replace waitForTimeout with proper expect().toHaveCount() in E2E tests

Addresses code review feedback on PR #12084
This commit is contained in:
openhands 2025-12-22 19:25:43 +00:00
parent da4b1e1a88
commit 71201ac4cd
5 changed files with 17 additions and 35 deletions

View File

@ -15,7 +15,7 @@ function InfiniteScrollTestComponent({
fetchNextPage,
threshold = 100,
}: InfiniteScrollTestComponentProps) {
const { ref } = useInfiniteScroll({
const { setRef } = useInfiniteScroll({
hasNextPage,
isFetchingNextPage,
fetchNextPage,
@ -25,7 +25,7 @@ function InfiniteScrollTestComponent({
return (
<div
data-testid="scroll-container"
ref={ref}
ref={setRef}
style={{ height: "200px", overflow: "auto" }}
>
<div style={{ height: "1000px" }}>Scrollable content</div>

View File

@ -66,7 +66,7 @@ export function ConversationPanel({ onClose }: ConversationPanelProps) {
const { mutate: updateConversation } = useUpdateConversation();
// Set up infinite scroll
const { ref: setScrollContainerRef } = useInfiniteScroll({
const { setRef: setScrollContainerRef } = useInfiniteScroll({
hasNextPage: !!hasNextPage,
isFetchingNextPage,
fetchNextPage,

View File

@ -21,7 +21,7 @@ export function RecentConversations() {
} = usePaginatedConversations(10);
// Set up infinite scroll
const { ref: setScrollContainerRef } = useInfiniteScroll({
const { setRef: setScrollContainerRef } = useInfiniteScroll({
hasNextPage: !!hasNextPage,
isFetchingNextPage,
fetchNextPage,

View File

@ -1,4 +1,4 @@
import { useEffect, useRef, useCallback, useState } from "react";
import { useEffect, useCallback, useState } from "react";
interface UseInfiniteScrollOptions {
hasNextPage: boolean;
@ -13,15 +13,8 @@ export const useInfiniteScroll = ({
fetchNextPage,
threshold = 100,
}: UseInfiniteScrollOptions) => {
const containerRef = useRef<HTMLDivElement>(null);
const [container, setContainer] = useState<HTMLDivElement | null>(null);
// Use a callback ref to track when the container is mounted
const setContainerRef = useCallback((node: HTMLDivElement | null) => {
containerRef.current = node;
setContainer(node);
}, []);
const handleScroll = useCallback(() => {
if (!container || isFetchingNextPage || !hasNextPage) {
return;
@ -44,5 +37,5 @@ export const useInfiniteScroll = ({
};
}, [container, handleScroll]);
return { ref: setContainerRef, containerRef };
return { setRef: setContainer };
};

View File

@ -37,16 +37,14 @@ test.describe("Infinite scroll for conversations", () => {
el.scrollTop = el.scrollHeight;
});
// Wait a bit for the infinite scroll to trigger and load more
await page.waitForTimeout(1000);
// Count conversations after scrolling
const afterScrollCount = await conversationCards.count();
// If there are more conversations available, the count should increase
// Wait for more conversations to load using proper assertion
// With 50 mock conversations and page size of 20, we should see more after scrolling
if (initialCount < 50) {
expect(afterScrollCount).toBeGreaterThan(initialCount);
await expect(conversationCards).toHaveCount(initialCount + 20, { timeout: 5000 }).catch(async () => {
// If exact count doesn't match, just verify we have more than initial
const afterScrollCount = await conversationCards.count();
expect(afterScrollCount).toBeGreaterThan(initialCount);
});
}
});
@ -66,31 +64,22 @@ test.describe("Infinite scroll for conversations", () => {
await expect(conversationCards.first()).toBeVisible();
// Count initial conversations (should be 3 by default)
const initialCount = await conversationCards.count();
expect(initialCount).toBe(3);
await expect(conversationCards).toHaveCount(3);
// Click "View More" to expand the list
const viewMoreButton = recentConversations.getByText("View More");
await expect(viewMoreButton).toBeVisible();
await viewMoreButton.click();
// Wait for the expansion animation
await page.waitForTimeout(500);
// Count conversations after expanding (should be 10)
const afterExpandCount = await conversationCards.count();
expect(afterExpandCount).toBe(10);
// Wait for conversations to expand to 10
await expect(conversationCards).toHaveCount(10);
// Click "View Less" to collapse the list
const viewLessButton = recentConversations.getByText("View Less");
await expect(viewLessButton).toBeVisible();
await viewLessButton.click();
// Wait for the collapse animation
await page.waitForTimeout(500);
// Count conversations after collapsing (should be back to 3)
const afterCollapseCount = await conversationCards.count();
expect(afterCollapseCount).toBe(3);
// Wait for conversations to collapse back to 3
await expect(conversationCards).toHaveCount(3);
});
});