From 914cf76d36c78bccc4ecfb7659af1a2c336b6a3f Mon Sep 17 00:00:00 2001 From: Sam-Rytech Date: Wed, 26 Aug 2026 23:19:31 +0100 Subject: [PATCH] fix(hooks): stable load identity, single mount fetch and in-flight cancellation Closes #442 `load` is already wrapped in useCallback and listed in the effect deps in both components, so ESLint react-hooks was already quiet. What was still unmet were the behaviours those deps are supposed to guarantee: ContractEventFeed - `live` was seeded from `pollInterval` at mount and never re-synced, so a feed mounted with polling off never started polling when the prop turned on. `pollInterval` is now synced during render, mirroring the existing `prevContractId` pattern. - Added a generation counter to `load`. Every call claims a generation and the counter is bumped when `contractId` changes, so a response for the previous contract is discarded instead of writing its events - or its loading state - over the current feed. - The polling effect now only owns the timer: it clears and re-arms on `pollInterval` changes, and nulls `intervalRef` on teardown. - Dropped the duplicate `setEvents([])` mount effect and its react-hooks/set-state-in-effect suppression; the render-phase reset above it already clears the previous contract's events. FeeEstimator - `onFeeLoad` is usually an inline arrow, so a new identity on every parent render rebuilt `load`, re-ran the effect and fired another request per render (and the callback typically sets parent state, so it fed itself). It is held in a ref now and `load` depends only on `client`, so mount performs exactly one request while the newest callback is still the one invoked. - Same generation guard, so a slow estimate cannot overwrite fresher data. Tests: both suites still mocked the removed `getClient` entrypoint (the client moved to SorokitContext), so they were red on main - a small `useSorokit` shim routes the hook at the same mock every existing test configures, and three export-button queries are matched against the current aria-label. New cases cover single fetch on mount, runtime pollInterval changes, timer re-arming, stale-response rejection and the inline-callback refetch loop. Co-Authored-By: Claude Opus 5 --- src/components/ContractEventFeed.test.tsx | 160 +++++++++++++++++++++- src/components/ContractEventFeed.tsx | 59 ++++++-- src/components/FeeEstimator.test.tsx | 76 ++++++++++ src/components/FeeEstimator.tsx | 29 +++- 4 files changed, 305 insertions(+), 19 deletions(-) diff --git a/src/components/ContractEventFeed.test.tsx b/src/components/ContractEventFeed.test.tsx index 99432b7..82b7490 100644 --- a/src/components/ContractEventFeed.test.tsx +++ b/src/components/ContractEventFeed.test.tsx @@ -10,6 +10,21 @@ vi.mock("@/lib/client", () => ({ getClient: vi.fn(), })); +// Issue #442 / context refactor: the components read their client from +// SorokitContext, so the hook is routed at the same `getClient` mock every test +// below configures. Without this the mocked client never reaches the component. +vi.mock("@/context/useSorokit", async () => { + const { getClient } = await import("@/lib/client"); + return { + useSorokit: () => ({ + client: getClient(), + isConnected: true, + address: "GTEST", + }), + }; +}); + + const CONTRACT_ID = "CAAZI4TCR3TY5OJHCTJC2A4QSY6CJWJH5IAJTGKIN2ER7LBNVKOCCWNA"; const MOCK_EVENT: ContractEvent = { @@ -400,7 +415,7 @@ describe("ContractEventFeed", () => { act(() => { vi.advanceTimersByTime(0); }); await waitFor(() => { - expect(screen.getByRole("button", { name: /export json/i })).toBeDisabled(); + expect(screen.getByRole("button", { name: /export \d+ events? as json/i })).toBeDisabled(); }); }); @@ -410,7 +425,7 @@ describe("ContractEventFeed", () => { act(() => { vi.advanceTimersByTime(0); }); await waitFor(() => screen.getByText("transfer")); - fireEvent.click(screen.getByRole("button", { name: /export json/i })); + fireEvent.click(screen.getByRole("button", { name: /export \d+ events? as json/i })); expect(mockCreateObjectURL).toHaveBeenCalledTimes(1); const blob = mockCreateObjectURL.mock.calls[0]![0] as Blob; @@ -433,7 +448,7 @@ describe("ContractEventFeed", () => { downloadName = this.download; }); - fireEvent.click(screen.getByRole("button", { name: /export json/i })); + fireEvent.click(screen.getByRole("button", { name: /export \d+ events? as json/i })); expect(clickSpy).toHaveBeenCalledTimes(1); expect(downloadName).toBe(`contract-events-${CONTRACT_ID}.json`); @@ -621,3 +636,142 @@ describe("ContractEventFeed", () => { }); }); }); + +// ── Issue #442: stale closures, single mount fetch, runtime poll changes ───── +describe("ContractEventFeed — issue #442", () => { + const OTHER_ID = "CBBB4TCR3TY5OJHCTJC2A4QSY6CJWJH5IAJTGKIN2ER7LBNVKOCCWNA"; + + function mockEvents(getEvents: ReturnType) { + vi.mocked(getClient).mockReturnValue({ + soroban: { getEvents }, + } as unknown as SorokitClient); + } + + function evt(id: string, topic: string): ContractEvent { + return { ...MOCK_EVENT, id, topics: [topic] }; + } + + beforeEach(() => { + vi.clearAllMocks(); + vi.useFakeTimers({ shouldAdvanceTime: true }); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("fires exactly one request on mount, even with polling enabled", async () => { + const getEvents = vi.fn().mockResolvedValue({ data: [], error: null }); + mockEvents(getEvents); + + render(); + act(() => { + vi.advanceTimersByTime(0); + }); + + await waitFor(() => expect(getEvents).toHaveBeenCalledTimes(1)); + + // Well short of the first poll — the polling effect must not have fetched. + act(() => { + vi.advanceTimersByTime(100); + }); + expect(getEvents).toHaveBeenCalledTimes(1); + }); + + it("starts polling when pollInterval goes from 0 to a positive value at runtime", async () => { + const getEvents = vi.fn().mockResolvedValue({ data: [], error: null }); + mockEvents(getEvents); + + const { rerender } = render( + , + ); + act(() => { + vi.advanceTimersByTime(0); + }); + await waitFor(() => expect(getEvents).toHaveBeenCalledTimes(1)); + + act(() => { + vi.advanceTimersByTime(3000); + }); + expect(getEvents).toHaveBeenCalledTimes(1); + + rerender(); + act(() => { + vi.advanceTimersByTime(1000); + }); + await waitFor(() => expect(getEvents).toHaveBeenCalledTimes(2)); + }); + + it("re-arms the timer at the new period when pollInterval changes", async () => { + const getEvents = vi.fn().mockResolvedValue({ data: [], error: null }); + mockEvents(getEvents); + + const { rerender } = render( + , + ); + act(() => { + vi.advanceTimersByTime(0); + }); + await waitFor(() => expect(getEvents).toHaveBeenCalledTimes(1)); + + act(() => { + vi.advanceTimersByTime(1000); + }); + await waitFor(() => expect(getEvents).toHaveBeenCalledTimes(2)); + + rerender(); + + // The old 1s timer must be gone… + act(() => { + vi.advanceTimersByTime(1000); + }); + expect(getEvents).toHaveBeenCalledTimes(2); + + // …and the new 5s one armed. + act(() => { + vi.advanceTimersByTime(4000); + }); + await waitFor(() => expect(getEvents).toHaveBeenCalledTimes(3)); + }); + + it("discards an in-flight response belonging to the previous contractId", async () => { + let resolveOld!: (value: { + data: ContractEvent[] | null; + error: string | null; + }) => void; + const oldPending = new Promise<{ + data: ContractEvent[] | null; + error: string | null; + }>((resolve) => { + resolveOld = resolve; + }); + + const getEvents = vi + .fn() + .mockReturnValueOnce(oldPending) + .mockResolvedValue({ data: [evt("evt-new", "NEW-EVT")], error: null }); + mockEvents(getEvents); + + const { rerender } = render(); + act(() => { + vi.advanceTimersByTime(0); + }); + await waitFor(() => expect(getEvents).toHaveBeenCalledTimes(1)); + + // Switch contracts while the first request is still in flight. + rerender(); + act(() => { + vi.advanceTimersByTime(0); + }); + await waitFor(() => expect(getEvents).toHaveBeenCalledTimes(2)); + expect(await screen.findByText("NEW-EVT")).toBeInTheDocument(); + + // The stale response lands late and must be dropped. + await act(async () => { + resolveOld({ data: [evt("evt-old", "OLD-EVT")], error: null }); + }); + + expect(screen.queryByText("OLD-EVT")).not.toBeInTheDocument(); + expect(screen.getByText("NEW-EVT")).toBeInTheDocument(); + }); +}); diff --git a/src/components/ContractEventFeed.tsx b/src/components/ContractEventFeed.tsx index 07a5f0c..cb50658 100644 --- a/src/components/ContractEventFeed.tsx +++ b/src/components/ContractEventFeed.tsx @@ -249,6 +249,11 @@ export function ContractEventFeed({ filterTypes ? new Set(filterTypes) : null, ); const intervalRef = useRef | null>(null); + // Issue #442: generation counter for `load`. Bumped on every call and + // whenever `contractId` changes, so a response that arrives after a newer + // request started - or after the feed moved to another contract - is + // discarded instead of overwriting the current events. + const requestIdRef = useRef(0); // IDs highlighted as newly-arrived. `prevEventIdsRef` is the baseline from // the previous successful load — `null` means no baseline yet, so the very @@ -270,12 +275,30 @@ export function ContractEventFeed({ setNewEventIds(new Set()); } + // Issue #442: `live` is seeded from `pollInterval` at mount, so a runtime + // change of the prop has to re-seed it - otherwise a feed mounted with + // polling off (pollInterval 0) never starts polling when the prop turns on. + // Synced during render, mirroring the `prevContractId` pattern above. + const [prevPollInterval, setPrevPollInterval] = useState(pollInterval); + if (prevPollInterval !== pollInterval) { + setPrevPollInterval(pollInterval); + setLive(pollInterval > 0); + } + useEffect(() => { prevEventIdsRef.current = null; + // Issue #442: invalidate whatever `load` has in flight for the previous + // contract. This effect is declared before the loading effect, so it runs + // first and the fresh load below gets the next generation number. + requestIdRef.current += 1; }, [contractId]); const load = useCallback(async () => { if (!contractId.trim() || !client) return; + // Issue #442: claim a generation up front; anything that resolves once a + // newer request exists is stale and must not touch state. + const requestId = ++requestIdRef.current; + const isStale = () => requestId !== requestIdRef.current; setLoading(true); try { const { data, error: err } = await client.soroban.getEvents( @@ -283,6 +306,7 @@ export function ContractEventFeed({ limit, fromLedger, ); + if (isStale()) return; if (err) { setError(err); setLoading(false); @@ -310,16 +334,19 @@ export function ContractEventFeed({ setError(null); setLastUpdatedAt(Date.now()); } catch (e) { + if (isStale()) return; setError(e instanceof Error ? e.message : "Failed to load events"); } finally { - setLoading(false); + // Issue #442: a stale call must not clear the spinner that belongs to the + // request that superseded it. + if (!isStale()) setLoading(false); } }, [client, contractId, limit, fromLedger]); - useEffect(() => { - // eslint-disable-next-line react-hooks/set-state-in-effect - setEvents([]); - }, [contractId]); + // Issue #442: the `setEvents([])` effect that used to sit here (behind a + // react-hooks/set-state-in-effect suppression) duplicated the render-phase + // reset above and ran again on mount. Removed - the render-phase reset + // already clears the previous contract's events without an extra pass. useEffect(() => { const timerId = window.setTimeout(() => { @@ -331,16 +358,24 @@ export function ContractEventFeed({ }; }, [load]); + // Issue #442: polling owns only the timer - the initial fetch belongs to the + // effect above, so mount fires exactly one request. Keyed on `pollInterval`, + // so changing the prop at runtime tears the old timer down and re-arms a new + // one at the new period. useEffect(() => { - if (live && pollInterval > 0 && contractId.trim() !== "") { - intervalRef.current = setInterval(() => { - void load(); - }, pollInterval); - } else { - if (intervalRef.current) clearInterval(intervalRef.current); + if (intervalRef.current) { + clearInterval(intervalRef.current); + intervalRef.current = null; } + if (!live || pollInterval <= 0 || contractId.trim() === "") return; + intervalRef.current = setInterval(() => { + void load(); + }, pollInterval); return () => { - if (intervalRef.current) clearInterval(intervalRef.current); + if (intervalRef.current) { + clearInterval(intervalRef.current); + intervalRef.current = null; + } }; }, [live, pollInterval, load, contractId]); diff --git a/src/components/FeeEstimator.test.tsx b/src/components/FeeEstimator.test.tsx index 594d38a..fa1b08c 100644 --- a/src/components/FeeEstimator.test.tsx +++ b/src/components/FeeEstimator.test.tsx @@ -7,6 +7,21 @@ vi.mock("@/lib/client", () => ({ getClient: vi.fn(), })); +// Issue #442 / context refactor: the components read their client from +// SorokitContext, so the hook is routed at the same `getClient` mock every test +// below configures. Without this the mocked client never reaches the component. +vi.mock("@/context/useSorokit", async () => { + const { getClient } = await import("@/lib/client"); + return { + useSorokit: () => ({ + client: getClient(), + isConnected: true, + address: "GTEST", + }), + }; +}); + + import type { SorokitClient } from "@/lib/client"; import { getClient } from "@/lib/client"; @@ -247,3 +262,64 @@ describe("FeeEstimator", { timeout: 15000 }, () => { }); }); }); + +// ── Issue #442: `load` identity must not depend on an inline onFeeLoad ─────── +describe("FeeEstimator — issue #442", () => { + function mockFee() { + const estimateFee = vi + .fn() + .mockResolvedValue({ data: { baseFee: "100", recommended: "500" }, error: null }); + vi.mocked(getClient).mockReturnValue({ + transaction: { estimateFee }, + } as unknown as SorokitClient); + return estimateFee; + } + + async function settle() { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 30)); + }); + } + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("makes exactly one request on mount", async () => { + const estimateFee = mockFee(); + render(); + await waitFor(() => expect(estimateFee).toHaveBeenCalledTimes(1)); + await settle(); + expect(estimateFee).toHaveBeenCalledTimes(1); + }); + + it("does not refetch when the parent re-renders with a new inline onFeeLoad", async () => { + const estimateFee = mockFee(); + const seen = vi.fn(); + + const { rerender } = render( seen(f)} />); + await waitFor(() => expect(estimateFee).toHaveBeenCalledTimes(1)); + + // A fresh arrow on each render used to rebuild `load` and refetch. + rerender( seen(f)} />); + rerender( seen(f)} />); + await settle(); + + expect(estimateFee).toHaveBeenCalledTimes(1); + }); + + it("calls the latest onFeeLoad, not the one captured at mount", async () => { + mockFee(); + const first = vi.fn(); + const second = vi.fn(); + + const { rerender } = render(); + await waitFor(() => expect(first).toHaveBeenCalledTimes(1)); + + rerender(); + fireEvent.click(screen.getByRole("button", { name: "Refresh fee estimate" })); + + await waitFor(() => expect(second).toHaveBeenCalledTimes(1)); + expect(first).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/components/FeeEstimator.tsx b/src/components/FeeEstimator.tsx index d232da1..ca9c55d 100644 --- a/src/components/FeeEstimator.tsx +++ b/src/components/FeeEstimator.tsx @@ -1,6 +1,6 @@ import { Refresh01Icon } from "@hugeicons/core-free-icons"; import { HugeiconsIcon } from "@hugeicons/react"; -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { Badge } from "@/components/ui/Badge"; import { Tooltip } from "@/components/ui/Tooltip"; @@ -32,12 +32,28 @@ export function FeeEstimator({ const [fee, setFee] = useState(null); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); + // Issue #442: `onFeeLoad` is normally an inline arrow, so it had a new + // identity on every parent render. As a `load` dependency that rebuilt + // `load`, re-ran the effect and fired another request per render (and the + // callback itself usually sets parent state, so it fed itself). Kept in a ref + // instead: the latest callback is always used, but `load` only depends on + // `client`, so mount performs exactly one request. + const onFeeLoadRef = useRef(onFeeLoad); + useEffect(() => { + onFeeLoadRef.current = onFeeLoad; + }, [onFeeLoad]); + // Issue #442: generation counter - an estimate that resolves after a newer + // one started is discarded rather than overwriting fresher data. + const requestIdRef = useRef(0); const load = useCallback(async () => { if (!client) return; + const requestId = ++requestIdRef.current; + const isStale = () => requestId !== requestIdRef.current; setLoading(true); try { const { data, error: err } = await client.transaction.estimateFee(); + if (isStale()) return; if (err) { setError(err); return; @@ -45,13 +61,18 @@ export function FeeEstimator({ setFee(data); setError(null); if (data) { - onFeeLoad?.(data); + onFeeLoadRef.current?.(data); } } finally { - setLoading(false); + // Issue #442: a stale call must not clear the spinner owned by the + // request that superseded it. + if (!isStale()) setLoading(false); } - }, [client, onFeeLoad]); + }, [client]); + // Issue #442: one effect owns both the initial fetch and the poll timer, so + // mount makes exactly one request and a changed `refreshInterval` re-arms the + // timer at the new period. useEffect(() => { const timerId = window.setTimeout(() => { void load();