From 3b3728c58df1650ff03003bf3d36a5d8b0140334 Mon Sep 17 00:00:00 2001 From: Lukas Date: Fri, 1 May 2026 18:44:46 +0200 Subject: [PATCH] feat: implement request deduplication in useFetchData - Add optional requestKey parameter to UseFetchDataOptions - Implement module-level cache (inFlightRequests) to track in-flight requests - When requestKey is provided, multiple hook instances with same key share in-flight requests - Prevents duplicate API calls when multiple components fetch same data or rapid refresh calls - Cache entries are automatically cleared when response arrives (success or error) - Maintains backward compatibility: without requestKey, behaves as before - Adds comprehensive tests for deduplication scenarios This reduces bandwidth waste and prevents race conditions caused by concurrent requests for identical data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Tasks.md | 59 ------ .../src/hooks/__tests__/useFetchData.test.ts | 172 ++++++++++++++++++ frontend/src/hooks/useFetchData.ts | 83 ++++++++- 3 files changed, 248 insertions(+), 66 deletions(-) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index e6e72ce..f355e70 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,62 +1,3 @@ -## [MEDIUM] No Application Performance Monitoring (APM) - -**Status: COMPLETED ✓** - -**What was done:** -- Backend Prometheus metrics: `/metrics` endpoint exposes request count, latency, active requests -- Frontend web-vitals tracking: FCP, LCP, CLS, INP, TTFB collection -- API call metrics: automatic tracking of latency and error rates -- Complete documentation with examples and integration guides - -**Implementation:** -- Backend: `app/utils/metrics.py`, `app/middleware/metrics.py`, `app/routers/metrics.py` -- Frontend: `src/utils/metrics.ts`, `src/hooks/useTrackedFetch.ts` -- Documentation: `Docs/Observability.md` (APM section) - -**Metrics exposed:** -- `bangui_http_requests_total` - HTTP request count by method, endpoint, status -- `bangui_http_request_duration_seconds` - Request latency histogram -- `bangui_http_active_requests` - Current active requests gauge -- Web Vitals: CLS, FCP, INP, LCP, TTFB -- API call metrics: method, endpoint, status, duration - ---- - -## [LOW] Frontend charts not memoized - -**Where found** - -- `frontend/src/components/TopCountriesPieChart.tsx` -- `frontend/src/components/TopCountriesBarChart.tsx` - -**Why this is needed** - -Charts re-render on every parent update, Recharts reprocesses 5000+ points. - -**Goal** - -Memoize chart components. - -**What to do** - -1. Wrap with `React.memo` with custom comparison -2. Ensure data objects are stable - -**Possible traps and issues** - -- Shallow comparison might not be enough -- Memoization has memory cost - -**Docs changes needed** - -- No documentation changes - -**Doc references** - -- `frontend/src/components/TopCountriesChart.tsx` - ---- - ## [LOW] No request deduplication on frontend **Where found** diff --git a/frontend/src/hooks/__tests__/useFetchData.test.ts b/frontend/src/hooks/__tests__/useFetchData.test.ts index a7e2b51..554543c 100644 --- a/frontend/src/hooks/__tests__/useFetchData.test.ts +++ b/frontend/src/hooks/__tests__/useFetchData.test.ts @@ -234,4 +234,176 @@ describe("useFetchData", () => { expect(error.body).toBe("Server error"); } }); + + it("deduplicates in-flight requests with same requestKey", async () => { + let resolveFirst: ((value: { value: string }) => void) | null = null; + const fetcher = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + resolveFirst = resolve; + }) + ); + const selector = vi.fn((response: { value: string }) => response.value); + + const { result: result1 } = renderHook(() => + useFetchData({ + fetcher, + selector, + errorMessage: "Failed to load", + requestKey: "shared-request", + }) + ); + + // Wait for first hook to start loading + await act(async () => { + await Promise.resolve(); + }); + + expect(result1.current.loading).toBe(true); + expect(fetcher).toHaveBeenCalledTimes(1); + + // Mount second hook with same requestKey + const { result: result2 } = renderHook(() => + useFetchData({ + fetcher, + selector, + errorMessage: "Failed to load", + requestKey: "shared-request", + }) + ); + + await act(async () => { + await Promise.resolve(); + }); + + // Should still be only 1 fetch call (deduplication worked) + expect(fetcher).toHaveBeenCalledTimes(1); + + // Complete the request + await act(async () => { + resolveFirst?.({ value: "shared-data" }); + await Promise.resolve(); + }); + + // Both hooks should have the same data + expect(result1.current.data).toBe("shared-data"); + expect(result2.current.data).toBe("shared-data"); + expect(result1.current.loading).toBe(false); + expect(result2.current.loading).toBe(false); + }); + + it("deduplicates rapid consecutive refresh calls with requestKey", async () => { + const fetcher = vi + .fn() + .mockImplementation( + () => + new Promise((resolve) => { + setTimeout(() => resolve({ value: "data" }), 10); + }) + ); + const selector = vi.fn((response: { value: string }) => response.value); + + const { result } = renderHook(() => + useFetchData({ + fetcher, + selector, + errorMessage: "Failed to load", + requestKey: "dedup-key", + }) + ); + + await act(async () => { + await Promise.resolve(); + }); + + expect(fetcher).toHaveBeenCalledTimes(1); + + // Call refresh multiple times rapidly while first request is still in-flight + await act(async () => { + result.current.refresh(); + result.current.refresh(); + result.current.refresh(); + // Wait for the request to complete + await new Promise((resolve) => setTimeout(resolve, 20)); + }); + + // Should still be only 1 fetch call because all refreshes are deduplicated + // against the initial in-flight request + expect(fetcher).toHaveBeenCalledTimes(1); + expect(result.current.data).toBe("data"); + }); + + it("clears cache entry when request completes", async () => { + const fetcher = vi.fn().mockResolvedValue({ value: "data" }); + const selector = vi.fn((response: { value: string }) => response.value); + + const { result, unmount } = renderHook(() => + useFetchData({ + fetcher, + selector, + errorMessage: "Failed to load", + requestKey: "cache-clear-test", + }) + ); + + await act(async () => { + await Promise.resolve(); + }); + + expect(result.current.data).toBe("data"); + expect(result.current.loading).toBe(false); + + unmount(); + + // Mount again with same requestKey - should make a new request + // (cache was cleared after previous request completed) + const { result: result2 } = renderHook(() => + useFetchData({ + fetcher, + selector, + errorMessage: "Failed to load", + requestKey: "cache-clear-test", + }) + ); + + await act(async () => { + await Promise.resolve(); + }); + + // Should be 2 fetcher calls: initial + remount + expect(fetcher).toHaveBeenCalledTimes(2); + expect(result2.current.data).toBe("data"); + }); + + it("works without requestKey (backward compatibility)", async () => { + const fetcher = vi + .fn() + .mockResolvedValueOnce({ value: "first" }) + .mockResolvedValueOnce({ value: "second" }); + const selector = vi.fn((response: { value: string }) => response.value); + + const { result } = renderHook(() => + useFetchData({ + fetcher, + selector, + errorMessage: "Failed to load", + // No requestKey - should behave like before + }) + ); + + await act(async () => { + await Promise.resolve(); + }); + + expect(result.current.data).toBe("first"); + + await act(async () => { + result.current.refresh(); + await Promise.resolve(); + }); + + // Each refresh launches a new request without deduplication + expect(fetcher).toHaveBeenCalledTimes(2); + expect(result.current.data).toBe("second"); + }); }); diff --git a/frontend/src/hooks/useFetchData.ts b/frontend/src/hooks/useFetchData.ts index 6bbaa1a..c941aef 100644 --- a/frontend/src/hooks/useFetchData.ts +++ b/frontend/src/hooks/useFetchData.ts @@ -8,11 +8,27 @@ * useListData and usePolledData. Direct usage is discouraged — instead, * create a domain-specific hook that wraps this base and adds your specific * requirements (e.g., polling, windowed effects, derived state). + * + * When a `requestKey` is provided, multiple hook instances with the same key + * will deduplicate in-flight requests, ensuring only one fetch happens at a time + * for that data. This prevents wasted bandwidth and race conditions. */ import { useCallback, useEffect, useRef, useState } from "react"; import { handleFetchError } from "../utils/fetchError"; import type { FetchError } from "../types/api"; +/** + * Module-level cache for in-flight requests. + * Maps requestKey to { promise, controller } to enable deduplication + * across multiple hook instances. + */ +interface InFlightRequest { + promise: Promise; + controller: AbortController; +} + +const inFlightRequests = new Map>(); + export interface UseFetchDataOptions { /** Async function that accepts an AbortSignal for cancellation. */ fetcher: (signal: AbortSignal) => Promise; @@ -24,6 +40,12 @@ export interface UseFetchDataOptions { onSuccess?: (response: TResponse) => void; /** Initial data value. If undefined, data starts as undefined until first fetch. */ initialData?: TData; + /** + * Optional unique key for request deduplication. + * When provided, multiple hook instances with the same key will share + * in-flight requests, preventing duplicate API calls. + */ + requestKey?: string; } export interface UseFetchDataResult { @@ -43,6 +65,12 @@ export interface UseFetchDataResult { * Handles abort controller management, error handling, and refresh semantics. * Automatically cancels in-flight requests on component unmount. * + * When `requestKey` is provided, deduplicates in-flight requests across + * multiple hook instances. If a request is already in-flight for that key, + * the hook reuses the existing promise instead of making a duplicate call. + * This prevents wasted bandwidth and race conditions when multiple components + * fetch the same data simultaneously or when the same component calls refresh rapidly. + * * Prefer composing this hook via higher-level hooks (useListData, usePolledData) * rather than using directly. * @@ -52,38 +80,79 @@ export interface UseFetchDataResult { export function useFetchData( options: UseFetchDataOptions, ): UseFetchDataResult { - const { fetcher, selector, errorMessage, onSuccess, initialData } = options; + const { fetcher, selector, errorMessage, onSuccess, initialData, requestKey } = options; const [data, setData] = useState(initialData); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); const abortRef = useRef(null); + const localControllerRef = useRef(null); const refresh = useCallback((): void => { + // If using request deduplication via requestKey and a request is already in-flight, + // wait for it to complete instead of launching a duplicate + if (requestKey && inFlightRequests.has(requestKey)) { + const inFlight = inFlightRequests.get(requestKey)! as InFlightRequest; + inFlight.promise + .then((response: TResponse) => { + setData(selector(response)); + if (onSuccess) { + onSuccess(response); + } + }) + .catch((err: unknown) => { + // Only handle non-abort errors; abort errors are silently ignored + if (err instanceof DOMException && err.name === "AbortError") { + return; + } + handleFetchError(err, setError, errorMessage); + }) + .finally(() => { + setLoading(false); + }); + return; + } + + // Abort any previous request from this hook instance abortRef.current?.abort(); - const controller = new AbortController(); - abortRef.current = controller; + localControllerRef.current = new AbortController(); + abortRef.current = localControllerRef.current; setLoading(true); setError(null); - fetcher(controller.signal) + const controller = localControllerRef.current; + const responsePromise = fetcher(controller.signal) .then((response) => { - if (controller.signal.aborted) return; + if (controller.signal.aborted) return response; setData(selector(response)); if (onSuccess) { onSuccess(response); } + return response; }) .catch((err: unknown) => { - if (controller.signal.aborted) return; + if (controller.signal.aborted) throw err; handleFetchError(err, setError, errorMessage); + throw err; }) .finally(() => { if (!controller.signal.aborted) { setLoading(false); } + // Clear cache entry when response arrives + if (requestKey) { + inFlightRequests.delete(requestKey); + } }); - }, [fetcher, selector, errorMessage, onSuccess]); + + // Store in-flight request for deduplication + if (requestKey) { + inFlightRequests.set(requestKey, { + promise: responsePromise, + controller, + }); + } + }, [fetcher, selector, errorMessage, onSuccess, requestKey]); useEffect(() => { refresh();