diff --git a/src/components/ContractEventFeed.test.tsx b/src/components/ContractEventFeed.test.tsx index 3a15959..881c0d5 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 = { @@ -454,7 +469,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; @@ -477,7 +492,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`); @@ -738,3 +753,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 97fa594..5f58845 100644 --- a/src/components/ContractEventFeed.tsx +++ b/src/components/ContractEventFeed.tsx @@ -250,7 +250,11 @@ export function ContractEventFeed({ filterTypes ? new Set(filterTypes) : null, ); const intervalRef = useRef | null>(null); - const [containerRef, isVisible] = useIsVisible(); + // 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 @@ -272,12 +276,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( @@ -285,6 +307,7 @@ export function ContractEventFeed({ limit, fromLedger, ); + if (isStale()) return; if (err) { setError(err); setLoading(false); @@ -312,16 +335,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(() => { @@ -333,6 +359,10 @@ 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(() => { // Dashboard keeps a visited screen mounted rather than unmounting it, // to preserve in-progress state — see the comment in Dashboard.tsx. @@ -348,8 +378,15 @@ export function ContractEventFeed({ } else { if (intervalRef.current) clearInterval(intervalRef.current); } + 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, isVisible, pollInterval, load, contractId]); diff --git a/src/components/FeeEstimator.test.tsx b/src/components/FeeEstimator.test.tsx index b790f42..83c8369 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"; @@ -309,3 +324,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 956217f..5de5eaf 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"; @@ -33,13 +33,28 @@ export function FeeEstimator({ const [fee, setFee] = useState(null); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); - const [containerRef, isVisible] = useIsVisible(); + // 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; @@ -47,13 +62,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(() => { // Dashboard keeps a visited screen mounted (rather than unmounting it) // to preserve in-progress state — see the comment in Dashboard.tsx.