useLeaderboard has no race-condition guard — rapid sort/page changes can let a stale response overwrite a newer one

[prodbycorne]

Problem

useLeaderboard (src/hooks/useLeaderboard.ts) triggers refresh() via a useCallback keyed on [offset, sortKey] (line 44-57), invoked from a useEffect on [refresh] (line 59-63) and again every REFRESH_MS (30s) on an interval. Each call to refresh() does:

fetchLeaderboard(offset, PAGE_SIZE, sortKey)
  .then(({ entries, total }) => { setEntries(entries); setTotal(total); ... })
  .catch(...)
  .finally(() => setIsLoading(false));

There is no AbortController, no request-id/sequence check, and no cancellation on unmount or on subsequent calls. If a user changes sortKey (via setSortKey, which also resets page to 1) and then quickly clicks to another page, or if the 30-second auto-refresh interval fires while a user-triggered refresh() from a sort/page change is still in flight, two overlapping fetchLeaderboard calls race. Whichever network response resolves last wins and overwrites entries/total/lastRefreshed, regardless of which request was actually issued more recently — a classic out-of-order-response bug. Because fetchLeaderboardFromEvents in src/lib/soroban.ts involves a Soroban RPC getEvents scan (not a fast, uniform-latency call), response times for different sort/offset combinations can vary significantly, making this race easy to hit in practice (e.g. switching sort key twice quickly, or paging while the background 30s refresh is mid-flight).

Acceptance Criteria

• Add request sequencing (an incrementing ref compared against the response, or AbortController wired through fetchLeaderboard) so only the response matching the latest request is applied to state.
• Cancel/ignore in-flight requests on unmount to avoid setting state on an unmounted component.
• Add a unit test that resolves two overlapping fetchLeaderboard promises out of order and asserts the hook's final state reflects the most recently issued request, not the most recently resolved one.
• Ensure the fix doesn't regress the existing 30s auto-refresh or the search debounce behavior.

Relevant Files

• src/hooks/useLeaderboard.ts — refresh() (~L44-57) has no cancellation/sequencing guard against overlapping requests
• src/lib/soroban.ts — getLeaderboard/fetchLeaderboardFromEvents has highly variable latency depending on whether the API or the RPC event-scan fallback is used, making the race easy to trigger

Additional Notes

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

Re-reading the hook top-to-bottom confirms the bug is exactly as described and finds one more contributing factor. The full sequence:

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]);

useEffect(() => {
  refresh();
  const id = setInterval(refresh, REFRESH_MS);
  return () => clearInterval(id);
}, [refresh]);

Because refresh is recreated whenever offset or sortKey changes, the useEffect above tears down and re-registers the 30s setInterval on every page/sort change — but it does not cancel any fetchLeaderboard promise that's already in flight from the previous refresh invocation. That in-flight promise keeps its .then/.catch/.finally handlers alive and will still call setEntries/setTotal/setIsLoading on the (still-mounted, but now representing a different offset/sortKey) component when it eventually resolves.

setSortKey (defined just below the snippet in the Relevant Files range) also resets page to 1 synchronously in the same tick as changing sortKey, so a user toggling sort twice in quick succession produces two overlapping requests whose offsets are both 0 but whose sortKey differs — the response body shape is identical ({entries, total}) so nothing structurally prevents the stale one from being applied silently.

Edge cases not yet covered by the acceptance criteria

1. Unmount during in-flight fetch. There is no cleanup flag checked before the .then callbacks run. If useLeaderboard unmounts (e.g. user navigates away from the leaderboard route) while a request is in flight, React will emit "Can't perform a React state update on an unmounted component" in dev, and in a suspense/concurrent context this can mask other bugs. Any AbortController-based fix should also gate the .then callbacks on a mountedRef/cleanup check, not just cancel the network request (fetch abort rejects the promise, but sorobanService.getLeaderboard's internal fallback path in fetchLeaderboardFromEvents swallows errors internally in some branches — see below — so an aborted request may not even reject cleanly).
2. The API-vs-event-scan fallback changes latency profile mid-session. SorobanService.getLeaderboard (src/lib/soroban.ts ~L1426-1439) tries fetchLeaderboardFromApi first and only falls back to fetchLeaderboardFromEvents (~L1467-1541) on a thrown error, logging console.warn('[SmartDrop] leaderboard API failed, falling back to event scan:', err). If the indexer API is flaky (intermittent 500s or timeouts), successive refresh() calls can alternate between the fast API path and the slow on-chain getEvents scan path (which itself calls getLatestLedger() then a getEvents RPC scan over LEADERBOARD_LOOKBACK_LEDGERS and aggregates in-memory), making the race trivially easy to reproduce in production without any user needing to double-click anything — a plain page load followed by an auto-refresh 30s later can race against a slow late-arriving retry.
3. searchQuery filtering happens client-side over entries (the paged derivation right after refresh), so a race that overwrites entries with a stale, different-sortKey page will also silently corrupt the search results shown to the user, not just the raw table — worth covering in the regression test.

Implementation sketch

const requestIdRef = useRef(0);

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

An incrementing ref is simpler to reason about here than threading an AbortController through sorobanService.getLeaderboard → fetchLeaderboardFromApi/fetchLeaderboardFromEvents, since the event-scan path makes multiple sequential RPC calls (getLatestLedger then getEvents) that would each need signal support and abort-aware error handling. The sequence-number approach gets correctness without touching soroban.ts at all, at the cost of not actually cancelling the underlying network request (acceptable for a read-only leaderboard scan, but worth calling out explicitly in the PR description since it means abandoned event-scans still consume RPC quota).

Testing strategy for reviewers

• The required unit test (two overlapping promises resolving out of order) should mock fetchLeaderboard directly (it's already exported for this reason) with two manually-controlled promises (e.g. via deferred() helpers) — resolve the second-issued one first and assert final state matches it, then resolve the first-issued one and assert state does not revert.
• Also verify the interval established in the useEffect still fires every REFRESH_MS after a sortKey/offset change (i.e., the fix doesn't accidentally cancel/duplicate the setInterval teardown, since refresh's identity changing is what currently re-triggers that effect).
• Verify the search debounce (SEARCH_DEBOUNCE_MS) still narrows paged correctly against whatever the latest entries are, per edge case 3 above.
• Cross-reference: issue No test exists for the leaderboard pagination/sort race condition where a stale response can overwrite a newer one #94 in this batch ("No test exists for the leaderboard pagination/sort race condition...") is asking for exactly the regression test this issue's acceptance criteria already requires — a PR closing useLeaderboard has no race-condition guard — rapid sort/page changes can let a stale response overwrite a newer one #79 should satisfy No test exists for the leaderboard pagination/sort race condition where a stale response can overwrite a newer one #94 as well; reviewers should check both are closed together rather than No test exists for the leaderboard pagination/sort race condition where a stale response can overwrite a newer one #94 being implemented as a separate, later PR against unfixed code.

[Dickson2015]

I would love to work on this issue I deliver very timely, I have worked on project like this. Thank you.

[grantfox-oss]

🦊 GrantFox — @Dickson2015 has been assigned to this issue as part of the Official Campaign | FWC26 campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #79)
2. Your PR will be reviewed by the SmartDropLabs maintainers

Good luck! Track your progress on GrantFox.

[ritaifeoluwa]

Closing as part of cleanup