Skip to content

Commit 0c38b32

Browse files
fix: resolve race conditions and add toast telemetry (#1209, #1211, #1212, #1214)
- useChatPagination (#1214): store setTimeout return in timerRef and clear on unmount to prevent state updates on unmounted components - useBeneficiaries (#1211): replace isMounted useState with useRef to eliminate the extra render cycle and the state-update-after-unmount race when components unmount before the mounted flag effect runs - useToast (#1209): wrap addToast/dismissToast/clearToasts with structured CustomEvent('toast_telemetry') dispatches so consumers can observe toast lifecycle events - useBridgeStats (#1212): add fetchIdRef generation counter so stale concurrent fetch results are discarded when a newer call supersedes them Add unit tests for each fix covering the unmount and concurrency paths. Closes #1209, #1211, #1212, #1214
1 parent fcd82d8 commit 0c38b32

8 files changed

Lines changed: 427 additions & 22 deletions

File tree

Dechat/dex_with_fiat_frontend/src/hooks/useBeneficiaries.test.ts

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { renderHook, act } from '@testing-library/react';
1+
import { renderHook, act, waitFor } from '@testing-library/react';
22
import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest';
33
import { useBeneficiaries } from './useBeneficiaries';
44

@@ -21,7 +21,7 @@ describe('useBeneficiaries', () => {
2121
});
2222

2323
afterEach(() => {
24-
// No timer cleanup needed for this hook
24+
vi.restoreAllMocks();
2525
});
2626

2727
it('loads beneficiaries from localStorage on mount', () => {
@@ -156,4 +156,65 @@ describe('useBeneficiaries', () => {
156156
expect(result.current.beneficiaries).toEqual(mockBeneficiaries);
157157
expect(result.current.isLoaded).toBe(true);
158158
});
159+
160+
it('does not update state after unmount when API fetch completes', async () => {
161+
let resolveFetch!: (value: Response) => void;
162+
const fetchPromise = new Promise<Response>((resolve) => {
163+
resolveFetch = resolve;
164+
});
165+
166+
vi.spyOn(global, 'fetch').mockReturnValue(fetchPromise);
167+
168+
const { result, unmount } = renderHook(() =>
169+
useBeneficiaries({ fetchFromApi: true, userId: 'user-1' }),
170+
);
171+
172+
expect(result.current.isLoaded).toBe(false);
173+
174+
// Unmount before the fetch resolves
175+
unmount();
176+
177+
// Resolve the fetch after unmount — should not update state
178+
resolveFetch(
179+
new Response(JSON.stringify([]), { status: 200 }),
180+
);
181+
182+
await waitFor(() => {
183+
// After resolving, state should remain unchanged (not loaded)
184+
// since the component was already unmounted
185+
expect(result.current.beneficiaries).toHaveLength(0);
186+
});
187+
});
188+
189+
it('cancels in-flight request when userId changes', async () => {
190+
let resolveFirst!: (value: Response) => void;
191+
const firstFetch = new Promise<Response>((resolve) => { resolveFirst = resolve; });
192+
let resolveSecond!: (value: Response) => void;
193+
const secondFetch = new Promise<Response>((resolve) => { resolveSecond = resolve; });
194+
195+
const fetchSpy = vi.spyOn(global, 'fetch')
196+
.mockReturnValueOnce(firstFetch)
197+
.mockReturnValueOnce(secondFetch);
198+
199+
let userId = 'user-a';
200+
const { result, rerender } = renderHook(() =>
201+
useBeneficiaries({ fetchFromApi: true, userId }),
202+
);
203+
204+
expect(fetchSpy).toHaveBeenCalledTimes(1);
205+
206+
userId = 'user-b';
207+
rerender();
208+
209+
// Resolve stale request after userId change — state should reflect second request result
210+
const firstData = [{ id: '1', name: 'Old', bankId: 1, bankName: 'B', bankCode: 'B', accountNumber: '1', accountName: 'A', createdAt: 0 }];
211+
resolveFirst(new Response(JSON.stringify(firstData), { status: 200 }));
212+
213+
const secondData = [{ id: '2', name: 'New', bankId: 2, bankName: 'C', bankCode: 'C', accountNumber: '2', accountName: 'B', createdAt: 0 }];
214+
resolveSecond(new Response(JSON.stringify(secondData), { status: 200 }));
215+
216+
await waitFor(() => {
217+
expect(result.current.isLoaded).toBe(true);
218+
});
219+
});
159220
});

Dechat/dex_with_fiat_frontend/src/hooks/useBeneficiaries.ts

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use client';
22

3-
import { useState, useEffect, useCallback } from 'react';
3+
import { useState, useEffect, useCallback, useRef } from 'react';
44

55
const KEYBOARD_SHORTCUTS = {
66
ADD_BENEFICIARY: 'Ctrl+B',
@@ -95,28 +95,31 @@ export function useBeneficiaries(options?: { fetchFromApi?: boolean; userId?: st
9595
const [beneficiaries, setBeneficiaries] = useState<Beneficiary[]>([]);
9696
const [isLoaded, setIsLoaded] = useState(false);
9797
const [selectedIndex, setSelectedIndex] = useState<number>(-1);
98-
const [isMounted, setIsMounted] = useState(false);
98+
const isMountedRef = useRef(false);
9999

100100
useEffect(() => {
101-
setIsMounted(true);
101+
isMountedRef.current = true;
102+
return () => {
103+
isMountedRef.current = false;
104+
};
102105
}, []);
103106

104107
// Fetch from API with deduplication if enabled
105108
useEffect(() => {
106-
if (!isMounted || !fetchFromApi || typeof window === 'undefined') return;
109+
if (!fetchFromApi || typeof window === 'undefined') return;
107110

108111
let cancelled = false;
109112

110113
(async () => {
111114
try {
112115
const data = await fetchBeneficiariesWithDedup(userId);
113-
if (!cancelled) {
116+
if (!cancelled && isMountedRef.current) {
114117
setBeneficiaries(Array.isArray(data) ? data : []);
115118
setIsLoaded(true);
116119
}
117120
} catch (error) {
118121
console.error('Error loading beneficiaries:', error);
119-
if (!cancelled) {
122+
if (!cancelled && isMountedRef.current) {
120123
setIsLoaded(true);
121124
}
122125
}
@@ -125,11 +128,11 @@ export function useBeneficiaries(options?: { fetchFromApi?: boolean; userId?: st
125128
return () => {
126129
cancelled = true;
127130
};
128-
}, [isMounted, fetchFromApi, userId]);
131+
}, [fetchFromApi, userId]);
129132

130133
// Load from localStorage if not fetching from API
131134
useEffect(() => {
132-
if (!isMounted || fetchFromApi || typeof window === 'undefined') return;
135+
if (fetchFromApi || typeof window === 'undefined') return;
133136
try {
134137
const stored = localStorage.getItem(STORAGE_KEY);
135138
if (stored) {
@@ -140,7 +143,7 @@ export function useBeneficiaries(options?: { fetchFromApi?: boolean; userId?: st
140143
setBeneficiaries([]);
141144
}
142145
setIsLoaded(true);
143-
}, [isMounted, fetchFromApi]);
146+
}, [fetchFromApi]);
144147

145148
useEffect(() => {
146149
if (!isLoaded || typeof window === 'undefined') return;
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
import { renderHook, act, waitFor } from '@testing-library/react';
2+
import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest';
3+
import useBridgeStats from './useBridgeStats';
4+
5+
vi.mock('@/lib/stellarContract', () => ({
6+
getContractBalance: vi.fn(),
7+
getBridgeLimit: vi.fn(),
8+
getTotalDeposited: vi.fn(),
9+
clearCache: vi.fn(),
10+
}));
11+
12+
import {
13+
getContractBalance,
14+
getBridgeLimit,
15+
getTotalDeposited,
16+
} from '@/lib/stellarContract';
17+
18+
const mockGetContractBalance = vi.mocked(getContractBalance);
19+
const mockGetBridgeLimit = vi.mocked(getBridgeLimit);
20+
const mockGetTotalDeposited = vi.mocked(getTotalDeposited);
21+
22+
describe('useBridgeStats', () => {
23+
beforeEach(() => {
24+
vi.useFakeTimers();
25+
mockGetContractBalance.mockResolvedValue(100n);
26+
mockGetBridgeLimit.mockResolvedValue(1000n);
27+
mockGetTotalDeposited.mockResolvedValue(500n);
28+
});
29+
30+
afterEach(() => {
31+
vi.useRealTimers();
32+
vi.clearAllMocks();
33+
});
34+
35+
it('fetches stats on mount and sets state', async () => {
36+
const { result } = renderHook(() => useBridgeStats());
37+
38+
await waitFor(() => {
39+
expect(result.current.loading).toBe(false);
40+
});
41+
42+
expect(result.current.balance).toBe(100n);
43+
expect(result.current.limit).toBe(1000n);
44+
expect(result.current.totalDeposited).toBe(500n);
45+
expect(result.current.fetchCount).toBe(1);
46+
expect(result.current.error).toBeNull();
47+
});
48+
49+
it('does not update state after unmount', async () => {
50+
let resolveBalance!: (v: bigint) => void;
51+
mockGetContractBalance.mockReturnValue(
52+
new Promise<bigint>((resolve) => { resolveBalance = resolve; }),
53+
);
54+
mockGetBridgeLimit.mockResolvedValue(1000n);
55+
mockGetTotalDeposited.mockResolvedValue(500n);
56+
57+
const { result, unmount } = renderHook(() => useBridgeStats());
58+
59+
unmount();
60+
61+
// Resolve after unmount — should not throw or update state
62+
expect(() => { resolveBalance(200n); }).not.toThrow();
63+
64+
await vi.runAllTimersAsync();
65+
66+
// State should remain at initial values since component was unmounted
67+
expect(result.current.balance).toBeNull();
68+
});
69+
70+
it('discards stale concurrent fetch result when a newer fetch supersedes it', async () => {
71+
let resolveFirst!: (v: bigint) => void;
72+
let resolveSecond!: (v: bigint) => void;
73+
74+
mockGetContractBalance
75+
.mockReturnValueOnce(new Promise<bigint>((r) => { resolveFirst = r; }))
76+
.mockReturnValueOnce(new Promise<bigint>((r) => { resolveSecond = r; }));
77+
mockGetBridgeLimit.mockResolvedValue(1000n);
78+
mockGetTotalDeposited.mockResolvedValue(500n);
79+
80+
const { result } = renderHook(() => useBridgeStats());
81+
82+
// Trigger a second fetch (manual refresh) before the first completes
83+
act(() => {
84+
void result.current.refetchStats();
85+
});
86+
87+
// Resolve the second (newer) fetch first
88+
resolveSecond(999n);
89+
90+
await waitFor(() => {
91+
expect(result.current.loading).toBe(false);
92+
});
93+
94+
// Now resolve the first (stale) fetch — its result should be discarded
95+
resolveFirst(111n);
96+
await vi.runAllTimersAsync();
97+
98+
// The newer fetch result (999n) should be preserved
99+
expect(result.current.balance).toBe(999n);
100+
});
101+
102+
it('sets error state when fetch fails', async () => {
103+
mockGetContractBalance.mockRejectedValue(new Error('Network error'));
104+
105+
const { result } = renderHook(() => useBridgeStats());
106+
107+
await waitFor(() => {
108+
expect(result.current.loading).toBe(false);
109+
});
110+
111+
expect(result.current.error).toBe('Network error');
112+
expect(result.current.balance).toBeNull();
113+
});
114+
115+
it('dispatches bridge_stats_telemetry events', async () => {
116+
const events: CustomEvent[] = [];
117+
window.addEventListener('bridge_stats_telemetry', (e) => {
118+
events.push(e as CustomEvent);
119+
});
120+
121+
const { unmount } = renderHook(() => useBridgeStats());
122+
123+
await waitFor(() => {
124+
expect(events.some((e) => e.detail?.event === 'bridge_stats_mounted')).toBe(true);
125+
});
126+
127+
await waitFor(() => {
128+
expect(events.some((e) => e.detail?.event === 'bridge_stats_fetch_success')).toBe(true);
129+
});
130+
131+
unmount();
132+
window.removeEventListener('bridge_stats_telemetry', (e) => {
133+
events.push(e as CustomEvent);
134+
});
135+
});
136+
});

Dechat/dex_with_fiat_frontend/src/hooks/useBridgeStats.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ export default function useBridgeStats(): BridgeStats {
3737
const [fetchCount, setFetchCount] = useState(0);
3838
const [lastFetchedAt, setLastFetchedAt] = useState<Date | null>(null);
3939
const isMountedRef = useRef(true);
40+
const fetchIdRef = useRef(0);
4041

4142
useEffect(() => {
4243
isMountedRef.current = true;
@@ -48,6 +49,7 @@ export default function useBridgeStats(): BridgeStats {
4849

4950
const refetchStats = useCallback(async () => {
5051
if (!isMountedRef.current) return;
52+
const fetchId = ++fetchIdRef.current;
5153
setLoading(true);
5254
setError(null);
5355
try {
@@ -56,20 +58,20 @@ export default function useBridgeStats(): BridgeStats {
5658
getBridgeLimit(),
5759
getTotalDeposited(),
5860
]);
59-
if (!isMountedRef.current) return;
61+
if (!isMountedRef.current || fetchId !== fetchIdRef.current) return;
6062
setBalance(b);
6163
setLimit(l);
6264
setTotalDeposited(t);
6365
setFetchCount((c) => c + 1);
6466
setLastFetchedAt(new Date());
6567
dispatchTelemetry('bridge_stats_fetch_success', { balance: b, limit: l });
6668
} catch (err) {
67-
if (!isMountedRef.current) return;
69+
if (!isMountedRef.current || fetchId !== fetchIdRef.current) return;
6870
const msg = err instanceof Error ? err.message : String(err);
6971
setError(msg);
7072
dispatchTelemetry('bridge_stats_fetch_error', { error: msg });
7173
} finally {
72-
if (isMountedRef.current) setLoading(false);
74+
if (isMountedRef.current && fetchId === fetchIdRef.current) setLoading(false);
7375
}
7476
}, []);
7577

Dechat/dex_with_fiat_frontend/src/hooks/useChatPagination.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,40 @@ describe('useChatPagination', () => {
5757
expect(result.current.visibleMessages).toHaveLength(10);
5858
expect(result.current.hasMore).toBe(false);
5959
});
60+
61+
it('does not update state after unmount when setTimeout fires', () => {
62+
const messages = createMessages(50);
63+
const { result, unmount } = renderHook(() => useChatPagination(messages, 20));
64+
65+
act(() => {
66+
result.current.loadMore();
67+
});
68+
69+
// Unmount before the 400ms timer fires
70+
unmount();
71+
72+
// Advancing the timer should not throw or warn about state updates on unmounted component
73+
expect(() => {
74+
act(() => {
75+
vi.advanceTimersByTime(500);
76+
});
77+
}).not.toThrow();
78+
});
79+
80+
it('isLoadingMore resets to false after loadMore completes', () => {
81+
const messages = createMessages(50);
82+
const { result } = renderHook(() => useChatPagination(messages, 20));
83+
84+
act(() => {
85+
result.current.loadMore();
86+
});
87+
88+
expect(result.current.isLoadingMore).toBe(true);
89+
90+
act(() => {
91+
vi.advanceTimersByTime(400);
92+
});
93+
94+
expect(result.current.isLoadingMore).toBe(false);
95+
});
6096
});

0 commit comments

Comments
 (0)