No test exists for the leaderboard pagination/sort race condition where a stale response can overwrite a newer one

[prodbycorne]

Problem

Companion to the useLeaderboard race-condition bug described elsewhere in this batch: there is currently no test in the repository that exercises out-of-order response resolution for useLeaderboard. Establishing this test before or alongside the fix ensures the race condition is actually verified to be fixed (rather than just 'probably fixed') and prevents a future refactor from silently reintroducing it. This is called out as its own issue because it requires constructing a deliberately-out-of-order mock (fetchLeaderboard resolving a later-issued request before an earlier one) which is a non-trivial test-authoring task in its own right — properly modeling the sequencing requires either fake timers or manually resolvable promises, not just vi.fn().mockResolvedValue(...).

Acceptance Criteria

• Add a test using manually-controlled/deferred promises (not simple mocked resolved values) for fetchLeaderboard that resolves a second, later-issued call before an earlier, first-issued call.
• Assert the hook's final entries/total/lastRefreshed state reflects the most recently issued request's data, not whichever resolved last.
• Add a companion test for the 30-second auto-refresh interval racing against a manual refresh() triggered by a user action (sort/page change).
• Ensure the test suite fails against the current (unfixed) implementation and passes once the race-condition fix from the companion issue lands — i.e. write this as a genuine regression test, not a tautological one.

Relevant Files

• src/hooks/useLeaderboard.ts — refresh() (~L44-57) is the function under test; currently has no protection against out-of-order responses
• src/app/leaderboard/page.tsx — the consumer whose displayed data would be affected by the race condition

Additional Notes

Confirmed against current source (src/hooks/useLeaderboard.ts, 97 lines)

refresh (L44-57) is a plain useCallback with no request-identity tracking whatsoever:

const refresh = useCallback(() => {
  setIsLoading(true);
  fetchLeaderboard(offset, PAGE_SIZE, sortKey)
    .then(({ entries, total }) => {
      setEntries(entries);
      setTotal(total);
      setLastRefreshed(new Date());
    })
    .catch(() => {
      setEntries([]);
      setTotal(0);
    })
    .finally(() => setIsLoading(false));
}, [offset, sortKey]);

There is no sequence number, no AbortController, and no comparison of "is this the response for the request I most recently issued." Every .then callback unconditionally calls setEntries/setTotal/setLastRefreshed regardless of whether a newer call to refresh has been issued since. refresh itself is re-created (new closure) whenever offset or sortKey changes (dependency array, L57), and the effect at L59-63 (useEffect(() => { refresh(); const id = setInterval(refresh, REFRESH_MS); ... }, [refresh])) re-fires immediately whenever refresh's identity changes — i.e., every sort/page change causes an immediate extra refresh() call on top of whatever the 30s interval (REFRESH_MS, L14) has already in flight. This is precisely the setup where a slow first request (e.g. triggered by an initial page load or previous sort) can resolve after a fast second request (e.g. triggered by a rapid follow-up sort click), and since neither .then checks staleness, the earlier-issued-but-later-resolving response wins and silently overwrites the newer, correct one.

Concrete repro shape for the test: call setSortKey('stake') then immediately setSortKey('credits') (or setPageState twice in a row) with fetchLeaderboard mocked so the first call's promise resolves after the second call's promise — assert the hook's final entries/total reflect the 'credits' (second, later-issued) result, not 'stake'.

The finally(() => setIsLoading(false)) compounds the bug

Beyond stale data winning, isLoading also has no request-identity guard: if request A (slow) is still in flight when request B (fast) resolves and sets isLoading(false), then when A finally resolves/rejects its own .finally will also call setIsLoading(false) — harmless in this specific case since both set it to the same value, but if a third overlapping request were mid-flight this compounds unpredictably with the entries/total bug: a user could see isLoading: false (looks settled) while stale data from an earlier request is what's actually displayed, i.e., no loading-spinner signal that anything is still wrong.

Auto-refresh vs. manual refresh race (AC #3)

The REFRESH_MS = 30_000 interval (L14) and the useEffect(() => { refresh(); const id = setInterval(refresh, REFRESH_MS); return () => clearInterval(id); }, [refresh]) (L59-63) mean that every time refresh's identity changes (any sort/page change, since those are in its dependency array), the interval is torn down and recreated with a fresh 30s countdown starting from that moment — so a user who manually triggers setSortKey/setPageState right before the auto-refresh was about to fire effectively delays the next auto-refresh by a full 30s. That's a separate, milder bug from the race condition itself (more a UX/staleness-window issue than a correctness bug), but the two interact: an auto-refresh tick landing in the small window between a user's setSortKey call and its resulting refresh() firing is the exact "manual refresh racing against auto-refresh" scenario the AC asks to be tested, and reproducing it requires careful fake-timer sequencing (advance timers partway, trigger the state change, advance the rest of the way) rather than a single vi.advanceTimersByTimeAsync(30000) call.

Search functionality is unaffected by the race (worth noting to avoid a wasted test)

searchQuery/paged (L65-69) filter over already-fetched entries client-side with no network request involved (debounced via L35-38's separate useEffect, unrelated to refresh), so there's no race to test there — search doesn't need coverage as part of this issue.

Implementation sketch for the companion fix (#79), informing what this test should assert once it lands

Simplest fix: a request-id ref (const requestIdRef = useRef(0)), incremented at the top of refresh before calling fetchLeaderboard, captured in a local variable, and checked inside .then/.catch before committing state:

const refresh = useCallback(() => {
  const requestId = ++requestIdRef.current;
  setIsLoading(true);
  fetchLeaderboard(offset, PAGE_SIZE, sortKey)
    .then(({ entries, total }) => {
      if (requestId !== requestIdRef.current) return; // stale, discard
      setEntries(entries);
      setTotal(total);
      setLastRefreshed(new Date());
    })
    .catch(() => {
      if (requestId !== requestIdRef.current) return;
      setEntries([]);
      setTotal(0);
    })
    .finally(() => {
      if (requestId === requestIdRef.current) setIsLoading(false);
    });
}, [offset, sortKey]);

This is a minimal, low-risk change that the regression test in this issue should be written to validate directly (assert stale responses are discarded, not merely that final state happens to look right).

Testing strategy

• Test file: new src/hooks/useLeaderboard.test.ts (none currently exists per the issue's own framing — confirm via find src/hooks -iname "*leaderboard*") or co-located alongside useSorobanEvents.test.ts's pattern for hook testing (renderHook from @/test/renderHook, vi.useFakeTimers()).
• Use manually-resolvable promises (let resolveFirst: (v: any) => void; const firstPromise = new Promise(r => { resolveFirst = r; }), same for resolveSecond) rather than mockResolvedValueOnce chains, since the whole point is controlling resolution order independent of call order — call refresh()/trigger a sort change to issue call Integrate Frontend with Deployed Soroban Contracts via RPC #1 (returns firstPromise), then trigger a second change issuing call Complete End-to-End Asset Locking Flow with Freighter Wallet #2 (returns secondPromise), then resolveSecond(...) before resolveFirst(...), and assert final hook state matches call Complete End-to-End Asset Locking Flow with Freighter Wallet #2's data.
• Mock fetchLeaderboard (exported at L17-23, wraps sorobanService.getLeaderboard) directly via vi.mock on the module, or inject a test double for sorobanService.getLeaderboard if fetchLeaderboard isn't independently mockable — check whether the hook imports fetchLeaderboard from the same module (it's defined in the same file, L17, and used at L46) which means mocking requires either vi.spyOn on the module's own export (awkward for same-file functions) or mocking sorobanService.getLeaderboard upstream instead — likely the cleaner seam.
• Write the test to fail against current main first (verify it actually goes red without the request-id guard) before/alongside the useLeaderboard has no race-condition guard — rapid sort/page changes can let a stale response overwrite a newer one #79 fix landing, per the issue's own AC Implement Global Error Handling with Toast Notifications #4 — this is worth being explicit about in the PR description so reviewers don't mistake a passing-by-coincidence test for a real regression guard.

Cross-reference

Direct companion to #79 ("useLeaderboard has no race-condition guard — rapid sort/page changes can let a stale response overwrite a newer one"), which is the implementation-side bug for the same refresh function (L44-57) described here. These should land together: #79's fix supplies the request-id (or AbortController) mechanism, and this issue's tests are what prove it actually works and stays working. Also touches the same consumer as #78 ("Leaderboard's 'You are rank X' banner only checks the currently-fetched page, not the user's true global rank") — connectedRank (L71-75) is derived from entries, so a stale-overwrite bug here could also transiently show an incorrect rank banner; not the same root cause as #78 but worth a shared mention in src/app/leaderboard/page.tsx if that page is touched for either fix.