diff --git a/ui/actions/finding-groups/finding-groups.test.ts b/ui/actions/finding-groups/finding-groups.test.ts new file mode 100644 index 0000000000..c73f9e8be5 --- /dev/null +++ b/ui/actions/finding-groups/finding-groups.test.ts @@ -0,0 +1,156 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// --------------------------------------------------------------------------- +// Hoisted mocks (must be declared before any imports that need them) +// --------------------------------------------------------------------------- + +const { fetchMock, getAuthHeadersMock, handleApiResponseMock } = vi.hoisted( + () => ({ + fetchMock: vi.fn(), + getAuthHeadersMock: vi.fn(), + handleApiResponseMock: vi.fn(), + }), +); + +vi.mock("@/lib", () => ({ + apiBaseUrl: "https://api.example.com/api/v1", + getAuthHeaders: getAuthHeadersMock, +})); + +vi.mock("@/lib/provider-filters", () => ({ + appendSanitizedProviderFilters: vi.fn(), +})); + +vi.mock("@/lib/server-actions-helper", () => ({ + handleApiResponse: handleApiResponseMock, +})); + +vi.mock("next/navigation", () => ({ + redirect: vi.fn(), +})); + +// --------------------------------------------------------------------------- +// Imports (after vi.mock declarations) +// --------------------------------------------------------------------------- + +import { + getFindingGroupResources, + getLatestFindingGroupResources, +} from "./finding-groups"; + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe("getFindingGroupResources — SSRF path traversal protection", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.stubGlobal("fetch", fetchMock); + getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" }); + handleApiResponseMock.mockResolvedValue({ data: [] }); + fetchMock.mockResolvedValue(new Response("", { status: 200 })); + }); + + it("should encode a normal checkId without alteration", async () => { + // Given + const checkId = "s3_bucket_public_access"; + + // When + await getFindingGroupResources({ checkId }); + + // Then — URL path must contain encoded checkId, not raw + const calledUrl = fetchMock.mock.calls[0][0] as string; + expect(calledUrl).toContain( + `/api/v1/finding-groups/${encodeURIComponent(checkId)}/resources`, + ); + }); + + it("should encode a checkId containing a forward slash (path traversal attempt)", async () => { + // Given — checkId with embedded slash: attacker attempts path traversal + const maliciousCheckId = "../../admin/secret"; + + // When + await getFindingGroupResources({ checkId: maliciousCheckId }); + + // Then — the URL must NOT contain a raw slash from the checkId + const calledUrl = fetchMock.mock.calls[0][0] as string; + const url = new URL(calledUrl); + // The path should NOT end in /resources with traversal segments between + expect(url.pathname).not.toContain("/admin/secret/resources"); + // The encoded checkId must appear in the path + expect(url.pathname).toContain( + `/finding-groups/${encodeURIComponent(maliciousCheckId)}/resources`, + ); + }); + + it("should encode a checkId containing %2F (URL-encoded slash traversal attempt)", async () => { + // Given — checkId with %2F: double-encoding traversal attempt + const maliciousCheckId = "foo%2Fbar"; + + // When + await getFindingGroupResources({ checkId: maliciousCheckId }); + + // Then + const calledUrl = fetchMock.mock.calls[0][0] as string; + const url = new URL(calledUrl); + expect(url.pathname).toContain( + `/finding-groups/${encodeURIComponent(maliciousCheckId)}/resources`, + ); + expect(url.pathname).not.toContain("/foo/bar/resources"); + }); + + it("should encode a checkId containing special chars like ? and #", async () => { + // Given + const maliciousCheckId = "check?admin=true#fragment"; + + // When + await getFindingGroupResources({ checkId: maliciousCheckId }); + + // Then + const calledUrl = fetchMock.mock.calls[0][0] as string; + expect(calledUrl).not.toContain("?admin=true"); + expect(calledUrl.split("?")[0]).toContain( + `/finding-groups/${encodeURIComponent(maliciousCheckId)}/resources`, + ); + }); +}); + +describe("getLatestFindingGroupResources — SSRF path traversal protection", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.stubGlobal("fetch", fetchMock); + getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" }); + handleApiResponseMock.mockResolvedValue({ data: [] }); + fetchMock.mockResolvedValue(new Response("", { status: 200 })); + }); + + it("should encode a normal checkId without alteration", async () => { + // Given + const checkId = "iam_user_mfa_enabled"; + + // When + await getLatestFindingGroupResources({ checkId }); + + // Then + const calledUrl = fetchMock.mock.calls[0][0] as string; + expect(calledUrl).toContain( + `/api/v1/finding-groups/latest/${encodeURIComponent(checkId)}/resources`, + ); + }); + + it("should encode a checkId containing a forward slash in the latest endpoint", async () => { + // Given + const maliciousCheckId = "../other-endpoint"; + + // When + await getLatestFindingGroupResources({ checkId: maliciousCheckId }); + + // Then + const calledUrl = fetchMock.mock.calls[0][0] as string; + const url = new URL(calledUrl); + expect(url.pathname).not.toContain("/other-endpoint/resources"); + expect(url.pathname).toContain( + `/finding-groups/latest/${encodeURIComponent(maliciousCheckId)}/resources`, + ); + }); +}); diff --git a/ui/actions/finding-groups/finding-groups.ts b/ui/actions/finding-groups/finding-groups.ts index a58a5ba5a4..25c1ba26e5 100644 --- a/ui/actions/finding-groups/finding-groups.ts +++ b/ui/actions/finding-groups/finding-groups.ts @@ -90,7 +90,9 @@ export const getFindingGroupResources = async ({ }) => { const headers = await getAuthHeaders({ contentType: false }); - const url = new URL(`${apiBaseUrl}/finding-groups/${checkId}/resources`); + const url = new URL( + `${apiBaseUrl}/finding-groups/${encodeURIComponent(checkId)}/resources`, + ); if (page) url.searchParams.append("page[number]", page.toString()); if (pageSize) url.searchParams.append("page[size]", pageSize.toString()); @@ -123,7 +125,7 @@ export const getLatestFindingGroupResources = async ({ const headers = await getAuthHeaders({ contentType: false }); const url = new URL( - `${apiBaseUrl}/finding-groups/latest/${checkId}/resources`, + `${apiBaseUrl}/finding-groups/latest/${encodeURIComponent(checkId)}/resources`, ); if (page) url.searchParams.append("page[number]", page.toString()); diff --git a/ui/actions/findings/findings-by-resource.test.ts b/ui/actions/findings/findings-by-resource.test.ts index bdc0765476..1e3f65246f 100644 --- a/ui/actions/findings/findings-by-resource.test.ts +++ b/ui/actions/findings/findings-by-resource.test.ts @@ -267,3 +267,77 @@ describe("resolveFindingIdsByVisibleGroupResources", () => { ); }); }); + +// --------------------------------------------------------------------------- +// Fix 4: Unbounded page[size] cap +// +// The bug: createResourceFindingResolutionUrl sets page[size]=resourceUids.length +// with no upper bound guard. The production fix adds Math.min(resourceUids.length, MAX_PAGE_SIZE) +// with MAX_PAGE_SIZE=500 as an explicit defensive cap. +// --------------------------------------------------------------------------- + +describe("resolveFindingIds — Fix 4: page[size] explicit cap at MAX_PAGE_SIZE=500", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.stubGlobal("fetch", fetchMock); + getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" }); + }); + + it("should use resourceUids.length as page[size] for a small batch (under 500)", async () => { + // Given — 3 resources, well under the cap + fetchMock.mockResolvedValue(new Response("", { status: 200 })); + handleApiResponseMock.mockResolvedValue({ + data: [{ id: "finding-1" }, { id: "finding-2" }, { id: "finding-3" }], + }); + + // When + await resolveFindingIds({ + checkId: "check-1", + resourceUids: ["resource-1", "resource-2", "resource-3"], + }); + + // Then — page[size] should equal the number of resourceUids (3) + const calledUrl = new URL(fetchMock.mock.calls[0][0]); + expect(calledUrl.searchParams.get("page[size]")).toBe("3"); + }); + + it("should cap page[size] at 500 when the chunk has exactly 500 UIDs (boundary value)", async () => { + // Given — exactly 500 unique UIDs (at the cap boundary) + const resourceUids = Array.from({ length: 500 }, (_, i) => `resource-${i}`); + fetchMock.mockResolvedValue(new Response("", { status: 200 })); + handleApiResponseMock.mockResolvedValue({ data: [] }); + + // When + await resolveFindingIds({ + checkId: "check-1", + resourceUids, + }); + + // Then — page[size] must be exactly 500 (not capped lower) + const firstUrl = new URL(fetchMock.mock.calls[0][0] as string); + expect(firstUrl.searchParams.get("page[size]")).toBe("500"); + }); + + it("should cap page[size] at 500 even when a chunk would exceed 500 — Math.min guard in URL builder", async () => { + // Given — 501 UIDs. The chunker splits into [500, 1]. + // The FIRST chunk has 500 UIDs → page[size] should be 500 (Math.min(500, 500)). + // The SECOND chunk has 1 UID → page[size] should be 1 (Math.min(1, 500)). + // This proves the Math.min cap fires correctly on every chunk. + const resourceUids = Array.from({ length: 501 }, (_, i) => `resource-${i}`); + fetchMock.mockResolvedValue(new Response("", { status: 200 })); + handleApiResponseMock.mockResolvedValue({ data: [] }); + + // When + await resolveFindingIds({ + checkId: "check-1", + resourceUids, + }); + + // Then — two fetch calls: one for 500 UIDs, one for 1 UID + expect(fetchMock).toHaveBeenCalledTimes(2); + const firstUrl = new URL(fetchMock.mock.calls[0][0] as string); + const secondUrl = new URL(fetchMock.mock.calls[1][0] as string); + expect(firstUrl.searchParams.get("page[size]")).toBe("500"); + expect(secondUrl.searchParams.get("page[size]")).toBe("1"); + }); +}); diff --git a/ui/actions/findings/findings-by-resource.ts b/ui/actions/findings/findings-by-resource.ts index 84ae5108f3..9d7afccb40 100644 --- a/ui/actions/findings/findings-by-resource.ts +++ b/ui/actions/findings/findings-by-resource.ts @@ -14,6 +14,9 @@ const FINDING_IDS_RESOLUTION_CONCURRENCY = 4; const FINDING_GROUP_RESOURCES_RESOLUTION_PAGE_SIZE = 500; const FINDING_FIELDS = "uid"; +/** Explicit upper bound for page[size] in resource-finding resolution requests. */ +const MAX_RESOURCE_FINDING_PAGE_SIZE = 500; + interface ResolveFindingIdsByCheckIdsParams { checkIds: string[]; filters?: Record; @@ -117,7 +120,10 @@ function createResourceFindingResolutionUrl({ url.searchParams.append("filter[check_id]", checkId); url.searchParams.append("filter[resource_uid__in]", resourceUids.join(",")); url.searchParams.append("filter[muted]", "false"); - url.searchParams.append("page[size]", resourceUids.length.toString()); + url.searchParams.append( + "page[size]", + Math.min(resourceUids.length, MAX_RESOURCE_FINDING_PAGE_SIZE).toString(), + ); appendSanitizedProviderTypeFilters(url, filters); diff --git a/ui/components/findings/table/inline-resource-container.test.tsx b/ui/components/findings/table/inline-resource-container.test.tsx new file mode 100644 index 0000000000..a6586bb4a0 --- /dev/null +++ b/ui/components/findings/table/inline-resource-container.test.tsx @@ -0,0 +1,313 @@ +/** + * Tests for inline-resource-container.tsx + * + * Fix 2: SSR crash — createPortal must be guarded by isMounted state + * Fix 3: Invalid Tailwind class — mt-[-10] → -mt-2.5 + */ + +import { act, render, screen } from "@testing-library/react"; +import type { + ComponentType, + HTMLAttributes, + ReactNode, + TdHTMLAttributes, +} from "react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// --------------------------------------------------------------------------- +// Mocks +// --------------------------------------------------------------------------- + +// Hoist createPortal spy so it is available when the vi.mock factory runs. +// vi.hoisted() runs before all imports, making the spy available in the factory. +const { createPortalSpy } = vi.hoisted(() => ({ + createPortalSpy: vi.fn(), +})); + +vi.mock("react-dom", async (importOriginal) => { + const original = await importOriginal(); + // Delegate to the real createPortal so other tests keep working, + // but allow spy assertions on call count and timing. + createPortalSpy.mockImplementation(original.createPortal); + return { + ...original, + createPortal: createPortalSpy, + }; +}); + +// Mock next/navigation before component import +vi.mock("next/navigation", () => ({ + useSearchParams: () => new URLSearchParams(), +})); + +// Mock heavy deps to avoid cascading import errors +vi.mock("@/actions/findings/findings-by-resource", () => ({ + resolveFindingIds: vi.fn().mockResolvedValue([]), +})); + +vi.mock("@/hooks/use-infinite-resources", () => ({ + useInfiniteResources: () => ({ + sentinelRef: vi.fn(), + refresh: vi.fn(), + loadMore: vi.fn(), + }), +})); + +vi.mock("@/hooks/use-scroll-hint", () => ({ + useScrollHint: () => ({ + containerRef: vi.fn(), + sentinelRef: vi.fn(), + showScrollHint: false, + }), +})); + +vi.mock("@/lib", () => ({ + hasDateOrScanFilter: vi.fn().mockReturnValue(false), +})); + +vi.mock("./column-finding-resources", () => ({ + getColumnFindingResources: vi.fn().mockReturnValue([]), +})); + +vi.mock("./findings-selection-context", () => ({ + FindingsSelectionContext: { + Provider: ({ children }: { children: ReactNode; value: unknown }) => ( + <>{children} + ), + }, +})); + +vi.mock("./resource-detail-drawer", () => ({ + ResourceDetailDrawer: () => ( +
Drawer
+ ), + useResourceDetailDrawer: () => ({ + isOpen: false, + isLoading: false, + isNavigating: false, + checkMeta: null, + currentIndex: 0, + totalResources: 0, + currentFinding: null, + otherFindings: [], + openDrawer: vi.fn(), + closeDrawer: vi.fn(), + navigatePrev: vi.fn(), + navigateNext: vi.fn(), + refetchCurrent: vi.fn(), + }), +})); + +vi.mock("@/components/shadcn/skeleton/skeleton", () => ({ + Skeleton: () =>
, +})); + +vi.mock("@/components/shadcn/spinner/spinner", () => ({ + Spinner: () =>
, +})); + +vi.mock("@/components/ui/table", () => ({ + TableCell: ({ + children, + ...props + }: { children?: ReactNode } & TdHTMLAttributes) => ( + {children} + ), + TableRow: ({ + children, + ...props + }: { children?: ReactNode } & HTMLAttributes) => ( + {children} + ), +})); + +// framer-motion: render children immediately +vi.mock("framer-motion", () => ({ + AnimatePresence: ({ children }: { children: ReactNode }) => <>{children}, + motion: { + div: ({ + children, + ...props + }: { children?: ReactNode } & HTMLAttributes) => ( +
{children}
+ ), + }, +})); + +// lucide-react +vi.mock("lucide-react", () => ({ + ChevronsDown: () => , +})); + +// @tanstack/react-table +vi.mock("@tanstack/react-table", () => ({ + flexRender: (Component: ComponentType | string, ctx: unknown) => { + if (typeof Component === "function") + return ; + return Component; + }, + getCoreRowModel: () => vi.fn(), + useReactTable: () => ({ + getRowModel: () => ({ rows: [] }), + getVisibleLeafColumns: () => [], + }), +})); + +// --------------------------------------------------------------------------- +// Imports (after mocks) +// --------------------------------------------------------------------------- + +import type { FindingGroupRow } from "@/types"; + +import { InlineResourceContainer } from "./inline-resource-container"; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +const mockGroup: FindingGroupRow = { + id: "group-1", + rowType: "group", + checkId: "s3_bucket_public_access", + checkTitle: "S3 Bucket Public Access Check", + resourcesTotal: 3, + resourcesFail: 3, + newCount: 0, + changedCount: 0, + mutedCount: 0, + severity: "high", + status: "FAIL", + providers: ["aws"], + updatedAt: "2024-01-01T00:00:00Z", +}; + +// --------------------------------------------------------------------------- +// Fix 2: SSR crash — portal only mounted after client-side mount +// --------------------------------------------------------------------------- + +describe("InlineResourceContainer — Fix 2: SSR portal guard", () => { + beforeEach(() => { + createPortalSpy.mockClear(); + }); + + it("should render without crash when document.body exists (JSDOM)", async () => { + // Given — JSDOM has document.body; this verifies the happy path + let renderError: Error | null = null; + + // When + try { + await act(async () => { + render( + + + + +
, + ); + }); + } catch (e) { + renderError = e as Error; + } + + // Then — component must not throw + expect(renderError).toBeNull(); + }); + + it("should render the portal content (ResourceDetailDrawer) after mount", async () => { + // Given + await act(async () => { + render( + + + + +
, + ); + }); + + // Then — drawer appears in the document via portal after mount + expect(screen.getByTestId("resource-detail-drawer")).toBeInTheDocument(); + }); + + it("should NOT call createPortal synchronously on initial render — only after useEffect fires (isMounted guard)", () => { + // Given — createPortalSpy is reset by beforeEach, so call count starts at 0. + // Before the fix: createPortal runs on the initial synchronous render → crash in SSR. + // After the fix: createPortal is guarded by isMounted (set via useEffect). + // useEffect fires AFTER commit, so createPortal must NOT be called + // during the synchronous render phase. + + // When — render inside synchronous act() and capture spy count INSIDE the callback. + // In React 19, act() DOES flush effects — but the callback runs BEFORE effects drain. + // So: the callback body executes first (synchronous render only), THEN act() flushes + // pending effects after the callback returns. + // Capturing spy state inside the callback captures the pre-effect state. + let portalCallsAfterSyncRender = 0; + act(() => { + render( + + + + +
, + ); + // Capture call count here — inside the callback = after synchronous render, + // but BEFORE act() drains pending effects (effects flush after this returns). + portalCallsAfterSyncRender = createPortalSpy.mock.calls.length; + }); + // After the callback returns, act() flushes pending effects (isMounted = true → re-render) + + // Then — createPortal must NOT have been called during the synchronous render phase + // (isMounted starts as false, createPortal is inside {isMounted && createPortal(...)}) + expect(portalCallsAfterSyncRender).toBe(0); + + // After act() completes, effects have flushed → isMounted = true → re-render → createPortal called + expect(createPortalSpy).toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// Fix 3: Invalid Tailwind class — -mt-2.5 instead of mt-[-10] +// --------------------------------------------------------------------------- + +describe("InlineResourceContainer — Fix 3: Valid Tailwind class", () => { + it("should use -mt-2.5 (valid Tailwind scale) on the inner resource table", async () => { + // Given + let container!: HTMLElement; + await act(async () => { + const result = render( + + + + +
, + ); + container = result.container; + }); + + // Then — the inner table element must have class "-mt-2.5", NOT "mt-[-10]" + const innerTable = container.querySelector("table table"); + expect(innerTable).not.toBeNull(); + expect(innerTable!.className).toContain("-mt-2.5"); + expect(innerTable!.className).not.toContain("mt-[-10]"); + }); +}); diff --git a/ui/components/findings/table/inline-resource-container.tsx b/ui/components/findings/table/inline-resource-container.tsx index 973f283cef..739275a048 100644 --- a/ui/components/findings/table/inline-resource-container.tsx +++ b/ui/components/findings/table/inline-resource-container.tsx @@ -10,7 +10,7 @@ import { import { AnimatePresence, motion } from "framer-motion"; import { ChevronsDown } from "lucide-react"; import { useSearchParams } from "next/navigation"; -import { useImperativeHandle, useRef, useState } from "react"; +import { useEffect, useImperativeHandle, useRef, useState } from "react"; import { createPortal } from "react-dom"; import { resolveFindingIds } from "@/actions/findings/findings-by-resource"; @@ -133,6 +133,11 @@ export function InlineResourceContainer({ const [rowSelection, setRowSelection] = useState({}); const [resources, setResources] = useState([]); const [isLoading, setIsLoading] = useState(true); + const [isMounted, setIsMounted] = useState(false); + + useEffect(() => { + setIsMounted(true); + }, []); // Scroll hint: shows "scroll for more" when content overflows const { @@ -308,7 +313,7 @@ export function InlineResourceContainer({ className="max-h-[440px] overflow-y-auto pl-6" > {/* Resource rows or skeleton placeholder */} - +
{isLoading && rows.length === 0 ? ( Array.from({ @@ -390,25 +395,26 @@ export function InlineResourceContainer({ - {createPortal( - { - if (!open) drawer.closeDrawer(); - }} - isLoading={drawer.isLoading} - isNavigating={drawer.isNavigating} - checkMeta={drawer.checkMeta} - currentIndex={drawer.currentIndex} - totalResources={drawer.totalResources} - currentFinding={drawer.currentFinding} - otherFindings={drawer.otherFindings} - onNavigatePrev={drawer.navigatePrev} - onNavigateNext={drawer.navigateNext} - onMuteComplete={handleDrawerMuteComplete} - />, - document.body, - )} + {isMounted && + createPortal( + { + if (!open) drawer.closeDrawer(); + }} + isLoading={drawer.isLoading} + isNavigating={drawer.isNavigating} + checkMeta={drawer.checkMeta} + currentIndex={drawer.currentIndex} + totalResources={drawer.totalResources} + currentFinding={drawer.currentFinding} + otherFindings={drawer.otherFindings} + onNavigatePrev={drawer.navigatePrev} + onNavigateNext={drawer.navigateNext} + onMuteComplete={handleDrawerMuteComplete} + />, + document.body, + )} ); } diff --git a/ui/hooks/use-infinite-resources.test.ts b/ui/hooks/use-infinite-resources.test.ts index 54c577d40f..1e9ac2f0ff 100644 --- a/ui/hooks/use-infinite-resources.test.ts +++ b/ui/hooks/use-infinite-resources.test.ts @@ -206,7 +206,9 @@ describe("useInfiniteResources", () => { // Then — only page 1 was fetched, never page 2 const calls = findingGroupActionsMock.getLatestFindingGroupResources.mock.calls; - const pageNumbers = calls.map((c: { page: number }[]) => c[0].page); + const pageNumbers = calls.map( + (c: unknown[]) => (c[0] as { page: number }).page, + ); expect(pageNumbers.every((p: number) => p === 1)).toBe(true); }); }); @@ -381,4 +383,157 @@ describe("useInfiniteResources", () => { ).toHaveBeenCalledWith(expect.objectContaining({ filters })); }); }); + + describe("when refresh() fires while loadNextPage is in-flight (race condition — Fix 5)", () => { + it("should discard in-flight page 2 and fetch page 1 when refresh fires during loadNextPage", async () => { + // Given — page 1 has 2 pages total, page 2 hangs indefinitely + const page1Response = makeApiResponse( + Array.from({ length: 10 }, (_, i) => ({ id: `r${i}` })), + { pages: 2 }, + ); + const page1Adapted = Array.from({ length: 10 }, (_, i) => + fakeResource(`r${i}`), + ); + + const page2Response = makeApiResponse( + Array.from({ length: 5 }, (_, i) => ({ id: `r${10 + i}` })), + { pages: 2 }, + ); + + const refreshPage1Response = makeApiResponse([{ id: "r-fresh-1" }], { + pages: 1, + }); + const refreshPage1Adapted = [fakeResource("r-fresh-1")]; + + // page 2 hangs until we explicitly resolve it + let resolveNextPage: (v: unknown) => void = () => {}; + const hangingPage2 = new Promise((r) => { + resolveNextPage = r; + }); + + let callCount = 0; + findingGroupActionsMock.getLatestFindingGroupResources.mockImplementation( + (args: { page: number }) => { + callCount++; + if (callCount === 1) { + return Promise.resolve(page1Response); + } + if (args.page === 2) { + return hangingPage2; + } + return Promise.resolve(refreshPage1Response); + }, + ); + + findingGroupActionsMock.adaptFindingGroupResourcesResponse + .mockReturnValueOnce(page1Adapted) + .mockReturnValue(refreshPage1Adapted); + + const onSetResources = vi.fn(); + const onAppendResources = vi.fn(); + + // When — mount and wait for page 1 + const { result } = renderHook(() => + useInfiniteResources( + defaultOptions({ onSetResources, onAppendResources }), + ), + ); + await flushAsync(); + + expect(onSetResources).toHaveBeenCalledWith(page1Adapted, true); + + // Trigger loadNextPage (increments pageRef to 2 in buggy code) + const sentinel = document.createElement("div"); + act(() => { + result.current.sentinelRef(sentinel); + }); + act(() => { + triggerIntersection(); + }); + // Do NOT flush — page 2 is hanging in-flight + + // Refresh fires while page 2 is in-flight + act(() => { + result.current.refresh(); + }); + await flushAsync(); + + // Resolve hanging page 2 after refresh (simulates late stale response) + await act(async () => { + resolveNextPage(page2Response); + await new Promise((r) => setTimeout(r, 0)); + }); + + // Then — the aborted page 2 must NOT deliver resources (signal.aborted check) + expect(onAppendResources).not.toHaveBeenCalled(); + + // The refresh must have fetched page 1 and delivered fresh resources + expect(onSetResources).toHaveBeenCalledWith(refreshPage1Adapted, false); + + // The refresh call must request page=1 (not page=3 due to stale pageRef) + // Exact call sequence: [0]=initial page 1, [1]=loadNextPage page 2, [2]=refresh page 1 + const calls = + findingGroupActionsMock.getLatestFindingGroupResources.mock.calls; + expect((calls[0][0] as { page: number }).page).toBe(1); // initial fetch + expect((calls[1][0] as { page: number }).page).toBe(2); // loadNextPage + expect((calls[2][0] as { page: number }).page).toBe(1); // refresh + }); + + it("should fetch sequential pages without skipping when loadNextPage is used normally", async () => { + // Given — page 1 has 3 pages; pages load sequentially + const makePageResponse = (startIdx: number, total: number) => + makeApiResponse( + Array.from({ length: 5 }, (_, i) => ({ id: `r${startIdx + i}` })), + { pages: total }, + ); + + findingGroupActionsMock.getLatestFindingGroupResources + .mockResolvedValueOnce(makePageResponse(0, 3)) // page 1 + .mockResolvedValueOnce(makePageResponse(5, 3)) // page 2 + .mockResolvedValueOnce(makePageResponse(10, 3)); // page 3 + + findingGroupActionsMock.adaptFindingGroupResourcesResponse + .mockReturnValueOnce( + Array.from({ length: 5 }, (_, i) => fakeResource(`r${i}`)), + ) + .mockReturnValueOnce( + Array.from({ length: 5 }, (_, i) => fakeResource(`r${5 + i}`)), + ) + .mockReturnValueOnce( + Array.from({ length: 5 }, (_, i) => fakeResource(`r${10 + i}`)), + ); + + const onAppendResources = vi.fn(); + + // When — mount and wait for page 1 + const { result } = renderHook(() => + useInfiniteResources(defaultOptions({ onAppendResources })), + ); + await flushAsync(); + + // Attach sentinel + const sentinel = document.createElement("div"); + act(() => { + result.current.sentinelRef(sentinel); + }); + + // Load page 2 + act(() => { + triggerIntersection(); + }); + await flushAsync(); + + // Load page 3 + act(() => { + triggerIntersection(); + }); + await flushAsync(); + + // Then — pages were fetched in order: 2, 3 (not 2, 4 due to double-increment) + const calls = + findingGroupActionsMock.getLatestFindingGroupResources.mock.calls; + expect(calls[1][0].page).toBe(2); + expect(calls[2][0].page).toBe(3); + }); + }); }); diff --git a/ui/hooks/use-infinite-resources.ts b/ui/hooks/use-infinite-resources.ts index b93acdd9c4..63a86aec3a 100644 --- a/ui/hooks/use-infinite-resources.ts +++ b/ui/hooks/use-infinite-resources.ts @@ -111,6 +111,11 @@ export function useInfiniteResources({ const totalPages = response?.meta?.pagination?.pages ?? 1; const hasMore = page < totalPages; + // Commit the page number only after a successful (non-aborted) fetch. + // This prevents a premature pageRef increment from loadNextPage being + // permanently committed if a concurrent abort fires before fetchPage + // starts executing. + pageRef.current = page; hasMoreRef.current = hasMore; if (append) { @@ -160,9 +165,11 @@ export function useInfiniteResources({ ) return; - const nextPage = pageRef.current + 1; - pageRef.current = nextPage; - fetchPage(nextPage, true, currentCheckIdRef.current, signal); + // Pass the next page number as an argument without pre-committing + // pageRef.current. The fetchPage function commits pageRef.current = page + // only after a successful (non-aborted) response, eliminating the race + // where a concurrent abort would leave pageRef permanently incremented. + fetchPage(pageRef.current + 1, true, currentCheckIdRef.current, signal); } // IntersectionObserver callback @@ -195,7 +202,6 @@ export function useInfiniteResources({ const controller = new AbortController(); controllerRef.current = controller; - pageRef.current = 1; hasMoreRef.current = true; isLoadingRef.current = false;