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>
This commit is contained in:
@@ -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
|
## [LOW] No request deduplication on frontend
|
||||||
|
|
||||||
**Where found**
|
**Where found**
|
||||||
|
|||||||
@@ -234,4 +234,176 @@ describe("useFetchData", () => {
|
|||||||
expect(error.body).toBe("Server error");
|
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");
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -8,11 +8,27 @@
|
|||||||
* useListData and usePolledData. Direct usage is discouraged — instead,
|
* useListData and usePolledData. Direct usage is discouraged — instead,
|
||||||
* create a domain-specific hook that wraps this base and adds your specific
|
* create a domain-specific hook that wraps this base and adds your specific
|
||||||
* requirements (e.g., polling, windowed effects, derived state).
|
* 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 { useCallback, useEffect, useRef, useState } from "react";
|
||||||
import { handleFetchError } from "../utils/fetchError";
|
import { handleFetchError } from "../utils/fetchError";
|
||||||
import type { FetchError } from "../types/api";
|
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<TResponse> {
|
||||||
|
promise: Promise<TResponse>;
|
||||||
|
controller: AbortController;
|
||||||
|
}
|
||||||
|
|
||||||
|
const inFlightRequests = new Map<string, InFlightRequest<unknown>>();
|
||||||
|
|
||||||
export interface UseFetchDataOptions<TResponse, TData> {
|
export interface UseFetchDataOptions<TResponse, TData> {
|
||||||
/** Async function that accepts an AbortSignal for cancellation. */
|
/** Async function that accepts an AbortSignal for cancellation. */
|
||||||
fetcher: (signal: AbortSignal) => Promise<TResponse>;
|
fetcher: (signal: AbortSignal) => Promise<TResponse>;
|
||||||
@@ -24,6 +40,12 @@ export interface UseFetchDataOptions<TResponse, TData> {
|
|||||||
onSuccess?: (response: TResponse) => void;
|
onSuccess?: (response: TResponse) => void;
|
||||||
/** Initial data value. If undefined, data starts as undefined until first fetch. */
|
/** Initial data value. If undefined, data starts as undefined until first fetch. */
|
||||||
initialData?: TData;
|
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<TData> {
|
export interface UseFetchDataResult<TData> {
|
||||||
@@ -43,6 +65,12 @@ export interface UseFetchDataResult<TData> {
|
|||||||
* Handles abort controller management, error handling, and refresh semantics.
|
* Handles abort controller management, error handling, and refresh semantics.
|
||||||
* Automatically cancels in-flight requests on component unmount.
|
* 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)
|
* Prefer composing this hook via higher-level hooks (useListData, usePolledData)
|
||||||
* rather than using directly.
|
* rather than using directly.
|
||||||
*
|
*
|
||||||
@@ -52,38 +80,79 @@ export interface UseFetchDataResult<TData> {
|
|||||||
export function useFetchData<TResponse, TData>(
|
export function useFetchData<TResponse, TData>(
|
||||||
options: UseFetchDataOptions<TResponse, TData>,
|
options: UseFetchDataOptions<TResponse, TData>,
|
||||||
): UseFetchDataResult<TData> {
|
): UseFetchDataResult<TData> {
|
||||||
const { fetcher, selector, errorMessage, onSuccess, initialData } = options;
|
const { fetcher, selector, errorMessage, onSuccess, initialData, requestKey } = options;
|
||||||
const [data, setData] = useState<TData | undefined>(initialData);
|
const [data, setData] = useState<TData | undefined>(initialData);
|
||||||
const [loading, setLoading] = useState(true);
|
const [loading, setLoading] = useState(true);
|
||||||
const [error, setError] = useState<FetchError | null>(null);
|
const [error, setError] = useState<FetchError | null>(null);
|
||||||
const abortRef = useRef<AbortController | null>(null);
|
const abortRef = useRef<AbortController | null>(null);
|
||||||
|
const localControllerRef = useRef<AbortController | null>(null);
|
||||||
|
|
||||||
const refresh = useCallback((): void => {
|
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<TResponse>;
|
||||||
|
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();
|
abortRef.current?.abort();
|
||||||
const controller = new AbortController();
|
localControllerRef.current = new AbortController();
|
||||||
abortRef.current = controller;
|
abortRef.current = localControllerRef.current;
|
||||||
|
|
||||||
setLoading(true);
|
setLoading(true);
|
||||||
setError(null);
|
setError(null);
|
||||||
|
|
||||||
fetcher(controller.signal)
|
const controller = localControllerRef.current;
|
||||||
|
const responsePromise = fetcher(controller.signal)
|
||||||
.then((response) => {
|
.then((response) => {
|
||||||
if (controller.signal.aborted) return;
|
if (controller.signal.aborted) return response;
|
||||||
setData(selector(response));
|
setData(selector(response));
|
||||||
if (onSuccess) {
|
if (onSuccess) {
|
||||||
onSuccess(response);
|
onSuccess(response);
|
||||||
}
|
}
|
||||||
|
return response;
|
||||||
})
|
})
|
||||||
.catch((err: unknown) => {
|
.catch((err: unknown) => {
|
||||||
if (controller.signal.aborted) return;
|
if (controller.signal.aborted) throw err;
|
||||||
handleFetchError(err, setError, errorMessage);
|
handleFetchError(err, setError, errorMessage);
|
||||||
|
throw err;
|
||||||
})
|
})
|
||||||
.finally(() => {
|
.finally(() => {
|
||||||
if (!controller.signal.aborted) {
|
if (!controller.signal.aborted) {
|
||||||
setLoading(false);
|
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(() => {
|
useEffect(() => {
|
||||||
refresh();
|
refresh();
|
||||||
|
|||||||
Reference in New Issue
Block a user