mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-25 18:40:06 -04:00
app: useLatestRequest reports loading before the debounce
`loading` only became true once the debounce timer fired, so for the first 300-500ms after every sendRequest the hook reported data=null, loading=false. Consumers branch on `loading` to choose between a spinner and an empty state, so the global search dropdown, the Zim search tab, and the map search rendered "No results" (or "No Zims have been indexed") for a search that had not run yet. Invariants now enforced and tested: * loading is true from sendRequest until the latest request settles. * The latest-request id is claimed synchronously in sendRequest, so a fetch already in flight is stale the moment a newer request is sent and cannot write data or clear loading during the new debounce. * A stale request that rejects late no longer nulls newer data. * A pending debounce is cancelled on unmount, and any in-flight fetch is treated as stale. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Roland
parent
bcd12c9b96
commit
5213ab93ea
2 changed files with 268 additions and 3 deletions
|
|
@ -195,6 +195,13 @@ export const useRecurringTimeout = (callback, delay) => {
|
|||
export const useLatestRequest = (delay = 300, defaultLoading = false) => {
|
||||
// A hook which ignores older requests and will only set `data` to the latest response's data.
|
||||
// usage: sendRequest(async () => await yourAPICall(...args));
|
||||
//
|
||||
// Invariants:
|
||||
// * `loading` is true from the moment `sendRequest` is called until the latest request settles,
|
||||
// including the debounce delay. Consumers use `loading` to choose between a spinner and an
|
||||
// empty state, so it must never be false while a request is pending.
|
||||
// * Only the latest request may write `data` or clear `loading`. A stale request that settles
|
||||
// late (resolve or reject) is ignored.
|
||||
|
||||
// The results from awaiting `fetchFunction`.
|
||||
const [data, setData] = React.useState(null);
|
||||
|
|
@ -207,10 +214,16 @@ export const useLatestRequest = (delay = 300, defaultLoading = false) => {
|
|||
if (debounceTimerRef.current) {
|
||||
clearTimeout(debounceTimerRef.current);
|
||||
}
|
||||
// Claim the latest id now, not when the debounce fires, so an earlier fetch that is still
|
||||
// in flight is already stale and cannot write `data` or clear `loading` while this
|
||||
// request is pending.
|
||||
const requestId = ++latestRequestRef.current;
|
||||
// Report the pending request before the debounce so the caller never shows "no results"
|
||||
// for a search that has not run yet.
|
||||
setLoading(true);
|
||||
|
||||
debounceTimerRef.current = setTimeout(async () => {
|
||||
setLoading(true);
|
||||
const requestId = ++latestRequestRef.current;
|
||||
debounceTimerRef.current = null;
|
||||
|
||||
try {
|
||||
const result = await fetchFunction();
|
||||
|
|
@ -220,7 +233,9 @@ export const useLatestRequest = (delay = 300, defaultLoading = false) => {
|
|||
}
|
||||
} catch (error) {
|
||||
console.error('Request failed:', error);
|
||||
setData(null);
|
||||
if (requestId === latestRequestRef.current) {
|
||||
setData(null);
|
||||
}
|
||||
} finally {
|
||||
if (requestId === latestRequestRef.current) {
|
||||
setLoading(false);
|
||||
|
|
@ -229,6 +244,18 @@ export const useLatestRequest = (delay = 300, defaultLoading = false) => {
|
|||
}, delay);
|
||||
}, [delay]);
|
||||
|
||||
// Do not fire a debounced request after the caller has unmounted, and treat any fetch still in
|
||||
// flight as stale so it does not set state on the unmounted hook.
|
||||
React.useEffect(() => {
|
||||
return () => {
|
||||
if (debounceTimerRef.current) {
|
||||
clearTimeout(debounceTimerRef.current);
|
||||
debounceTimerRef.current = null;
|
||||
}
|
||||
latestRequestRef.current++;
|
||||
};
|
||||
}, []);
|
||||
|
||||
return {data, sendRequest, loading};
|
||||
};
|
||||
|
||||
|
|
|
|||
238
app/src/hooks/useLatestRequest.test.js
Normal file
238
app/src/hooks/useLatestRequest.test.js
Normal file
|
|
@ -0,0 +1,238 @@
|
|||
import {act, renderHook} from '@testing-library/react';
|
||||
import {useLatestRequest} from './customHooks';
|
||||
|
||||
// Invariants of useLatestRequest:
|
||||
// 1. From the moment sendRequest is called until the latest request settles, `loading` is true.
|
||||
// Consumers branch on `loading` to decide between a spinner and an empty state, so a window
|
||||
// where data is null and loading is false renders "No results" for a search that has not run.
|
||||
// 2. Only the latest request may write `data` or clear `loading`; a stale request that resolves
|
||||
// or rejects after a newer one was sent must not touch either.
|
||||
|
||||
const DELAY = 300;
|
||||
|
||||
// A fetch function whose promise the test settles by hand.
|
||||
const deferred = () => {
|
||||
let resolve, reject;
|
||||
const promise = new Promise((res, rej) => {
|
||||
resolve = res;
|
||||
reject = rej;
|
||||
});
|
||||
return {promise, resolve, reject, fn: jest.fn(() => promise)};
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
jest.useFakeTimers();
|
||||
jest.spyOn(console, 'error').mockImplementation(() => {
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
jest.useRealTimers();
|
||||
console.error.mockRestore();
|
||||
});
|
||||
|
||||
describe('useLatestRequest loading window', () => {
|
||||
test('loading is true synchronously after sendRequest, before the debounce elapses', () => {
|
||||
const {result} = renderHook(() => useLatestRequest(DELAY));
|
||||
expect(result.current.loading).toBe(false);
|
||||
expect(result.current.data).toBeNull();
|
||||
|
||||
const req = deferred();
|
||||
act(() => {
|
||||
result.current.sendRequest(req.fn);
|
||||
});
|
||||
|
||||
// The debounce has not fired yet: the fetch has not been called, but the hook must
|
||||
// already report that a request is pending.
|
||||
expect(req.fn).not.toHaveBeenCalled();
|
||||
expect(result.current.loading).toBe(true);
|
||||
expect(result.current.data).toBeNull();
|
||||
});
|
||||
|
||||
test('loading stays true through the debounce and the fetch, then data arrives', async () => {
|
||||
const {result} = renderHook(() => useLatestRequest(DELAY));
|
||||
const req = deferred();
|
||||
act(() => {
|
||||
result.current.sendRequest(req.fn);
|
||||
});
|
||||
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(DELAY);
|
||||
});
|
||||
expect(req.fn).toHaveBeenCalledTimes(1);
|
||||
expect(result.current.loading).toBe(true);
|
||||
|
||||
await act(async () => {
|
||||
req.resolve({results: [1, 2]});
|
||||
});
|
||||
expect(result.current.loading).toBe(false);
|
||||
expect(result.current.data).toEqual({results: [1, 2]});
|
||||
});
|
||||
|
||||
test('loading stays true when a second sendRequest resets the debounce', () => {
|
||||
const {result} = renderHook(() => useLatestRequest(DELAY));
|
||||
const first = deferred();
|
||||
const second = deferred();
|
||||
|
||||
act(() => {
|
||||
result.current.sendRequest(first.fn);
|
||||
});
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(DELAY - 1);
|
||||
});
|
||||
act(() => {
|
||||
result.current.sendRequest(second.fn);
|
||||
});
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(DELAY - 1);
|
||||
});
|
||||
|
||||
// Neither request has fired, and the user is still waiting.
|
||||
expect(first.fn).not.toHaveBeenCalled();
|
||||
expect(second.fn).not.toHaveBeenCalled();
|
||||
expect(result.current.loading).toBe(true);
|
||||
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(1);
|
||||
});
|
||||
expect(first.fn).not.toHaveBeenCalled();
|
||||
expect(second.fn).toHaveBeenCalledTimes(1);
|
||||
expect(result.current.loading).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('useLatestRequest stale requests', () => {
|
||||
// Fire two requests so both fetches are in flight, returning their deferreds.
|
||||
const sendTwo = (result) => {
|
||||
const first = deferred();
|
||||
const second = deferred();
|
||||
act(() => {
|
||||
result.current.sendRequest(first.fn);
|
||||
});
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(DELAY);
|
||||
});
|
||||
act(() => {
|
||||
result.current.sendRequest(second.fn);
|
||||
});
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(DELAY);
|
||||
});
|
||||
expect(first.fn).toHaveBeenCalledTimes(1);
|
||||
expect(second.fn).toHaveBeenCalledTimes(1);
|
||||
return {first, second};
|
||||
};
|
||||
|
||||
test('a stale request that resolves late does not overwrite newer data or clear loading', async () => {
|
||||
const {result} = renderHook(() => useLatestRequest(DELAY));
|
||||
const {first, second} = sendTwo(result);
|
||||
|
||||
await act(async () => {
|
||||
first.resolve('stale');
|
||||
});
|
||||
expect(result.current.data).toBeNull();
|
||||
expect(result.current.loading).toBe(true);
|
||||
|
||||
await act(async () => {
|
||||
second.resolve('latest');
|
||||
});
|
||||
expect(result.current.data).toBe('latest');
|
||||
expect(result.current.loading).toBe(false);
|
||||
});
|
||||
|
||||
test('a stale request that rejects late does not clear newer data', async () => {
|
||||
const {result} = renderHook(() => useLatestRequest(DELAY));
|
||||
const {first, second} = sendTwo(result);
|
||||
|
||||
await act(async () => {
|
||||
second.resolve('latest');
|
||||
});
|
||||
expect(result.current.data).toBe('latest');
|
||||
expect(result.current.loading).toBe(false);
|
||||
|
||||
await act(async () => {
|
||||
first.reject(new Error('slow failure'));
|
||||
});
|
||||
expect(result.current.data).toBe('latest');
|
||||
expect(result.current.loading).toBe(false);
|
||||
});
|
||||
|
||||
// Send A and let its fetch start, then send B and leave B's debounce pending. B has claimed
|
||||
// "latest" the moment it was sent, so A is stale even though B has not fetched yet.
|
||||
const sendDuringFlight = (result) => {
|
||||
const first = deferred();
|
||||
const second = deferred();
|
||||
act(() => {
|
||||
result.current.sendRequest(first.fn);
|
||||
});
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(DELAY);
|
||||
});
|
||||
expect(first.fn).toHaveBeenCalledTimes(1);
|
||||
act(() => {
|
||||
result.current.sendRequest(second.fn);
|
||||
});
|
||||
expect(second.fn).not.toHaveBeenCalled();
|
||||
return {first, second};
|
||||
};
|
||||
|
||||
test.each([
|
||||
['resolves', (req) => req.resolve('stale')],
|
||||
['rejects', (req) => req.reject(new Error('slow failure'))],
|
||||
])('an in-flight request that %s during a newer debounce neither writes data nor clears loading',
|
||||
async (_label, settle) => {
|
||||
const {result} = renderHook(() => useLatestRequest(DELAY));
|
||||
const {first, second} = sendDuringFlight(result);
|
||||
|
||||
await act(async () => {
|
||||
settle(first);
|
||||
});
|
||||
expect(result.current.data).toBeNull();
|
||||
expect(result.current.loading).toBe(true);
|
||||
|
||||
// B's debounce fires and B fetches; still loading until B settles.
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(DELAY);
|
||||
});
|
||||
expect(second.fn).toHaveBeenCalledTimes(1);
|
||||
expect(result.current.loading).toBe(true);
|
||||
|
||||
await act(async () => {
|
||||
second.resolve('latest');
|
||||
});
|
||||
expect(result.current.data).toBe('latest');
|
||||
expect(result.current.loading).toBe(false);
|
||||
});
|
||||
|
||||
test('the latest request rejecting clears loading and data', async () => {
|
||||
const {result} = renderHook(() => useLatestRequest(DELAY));
|
||||
const req = deferred();
|
||||
act(() => {
|
||||
result.current.sendRequest(req.fn);
|
||||
});
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(DELAY);
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
req.reject(new Error('failure'));
|
||||
});
|
||||
expect(result.current.loading).toBe(false);
|
||||
expect(result.current.data).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('useLatestRequest unmount', () => {
|
||||
test('a pending debounce does not fire after unmount', () => {
|
||||
const {result, unmount} = renderHook(() => useLatestRequest(DELAY));
|
||||
const req = deferred();
|
||||
act(() => {
|
||||
result.current.sendRequest(req.fn);
|
||||
});
|
||||
unmount();
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(DELAY);
|
||||
});
|
||||
expect(req.fn).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue