From d317ae75aa720ccb9f742ac2c7d26b10a67c559d Mon Sep 17 00:00:00 2001 From: Tomeshwari-02 <179694969+Tomeshwari-02@users.noreply.github.com> Date: Sun, 16 Aug 2026 11:32:44 +0530 Subject: [PATCH] Fix data hooks and adapter edge cases --- packages/adapters/src/execa/index.ts | 11 ++-- .../adapters/src/localStorage/index.test.ts | 3 + packages/adapters/src/localStorage/index.ts | 7 +++ packages/data/src/hooks.ts | 1 + packages/data/src/hooks/useFileWatch.test.ts | 15 +++++ packages/data/src/hooks/useFileWatch.ts | 3 + packages/data/src/hooks/useMutation.test.ts | 59 ++++++++++++++++++- packages/data/src/hooks/useMutation.ts | 42 +++++++++---- packages/data/src/hooks/usePolling.test.ts | 17 ++++++ packages/data/src/hooks/usePolling.ts | 15 +++-- packages/data/src/hooks/useSSE.test.ts | 23 +++++++- packages/data/src/hooks/useSSE.ts | 5 +- packages/data/src/http.test.ts | 2 + packages/data/src/http.ts | 27 +++++---- packages/data/src/useWebSocket.test.tsx | 16 ++++- 15 files changed, 210 insertions(+), 36 deletions(-) diff --git a/packages/adapters/src/execa/index.ts b/packages/adapters/src/execa/index.ts index d626eb5fe..2b7eba9bc 100644 --- a/packages/adapters/src/execa/index.ts +++ b/packages/adapters/src/execa/index.ts @@ -3,7 +3,8 @@ // ───────────────────────────────────────────────────── import type { Options } from 'execa'; -import { execa } from 'execa'; + +type ExecaFunction = (file: string, args?: string[], options?: Options) => any; export interface UseExecaResult { run( @@ -32,15 +33,15 @@ async function getExeca(): Promise { } } -function isExecaCallable(obj: unknown): obj is typeof execa { +function isExecaCallable(obj: unknown): obj is ExecaFunction { return typeof obj === 'function'; } -function isExecaModuleWithExeca(obj: unknown): obj is { execa: typeof execa } { +function isExecaModuleWithExeca(obj: unknown): obj is { execa: ExecaFunction } { return typeof obj === 'object' && obj !== null && 'execa' in obj && typeof (obj as { execa: unknown }).execa === 'function'; } -function isExecaModuleWithDefault(obj: unknown): obj is { default: typeof execa } { +function isExecaModuleWithDefault(obj: unknown): obj is { default: ExecaFunction } { return typeof obj === 'object' && obj !== null && 'default' in obj && typeof (obj as { default: unknown }).default === 'function'; } @@ -60,7 +61,7 @@ export function useExeca(globalOpts?: Options): UseExecaResult { opts?: Options ): AsyncGenerator { const execaModule: unknown = await getExeca(); - let execaFn: typeof execa; + let execaFn: ExecaFunction; if (isExecaModuleWithExeca(execaModule)) { execaFn = execaModule.execa; diff --git a/packages/adapters/src/localStorage/index.test.ts b/packages/adapters/src/localStorage/index.test.ts index f819135e0..f6313ac97 100644 --- a/packages/adapters/src/localStorage/index.test.ts +++ b/packages/adapters/src/localStorage/index.test.ts @@ -35,4 +35,7 @@ describe('useLocalStorage', () => { store.set('a', 1); store.set('b', 2); store.clear() expect(store.get('a')).toBeNull(); expect(store.get('b')).toBeNull() }) + it('rejects service names that can escape the config directory', () => { + expect(() => useLocalStorage('../outside-termui')).toThrow(/service must contain/) + }) }) diff --git a/packages/adapters/src/localStorage/index.ts b/packages/adapters/src/localStorage/index.ts index 0f7aeb806..87661a66c 100644 --- a/packages/adapters/src/localStorage/index.ts +++ b/packages/adapters/src/localStorage/index.ts @@ -9,7 +9,14 @@ export interface LocalStorageAdapter { clear(): void } +function assertValidServiceName(service: string): void { + if (!/^[A-Za-z0-9][A-Za-z0-9._-]*$/.test(service)) { + throw new Error('useLocalStorage() service must contain only letters, numbers, dots, underscores, and hyphens, and must start with a letter or number.') + } +} + function resolveStorePath(service: string): string { + assertValidServiceName(service) const dir = path.join(os.homedir(), '.config', 'termui') if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true }) return path.join(dir, `${service}.json`) diff --git a/packages/data/src/hooks.ts b/packages/data/src/hooks.ts index bf84a1ec7..ef12a9cb3 100644 --- a/packages/data/src/hooks.ts +++ b/packages/data/src/hooks.ts @@ -265,6 +265,7 @@ export function useWebSocket(url: string): UseWebSocketReturn { let isMounted = true; const thisGeneration = ++generationRef.current; retryCountRef.current = 0; + setMessage(null); function connect() { if (socketRef.current) { diff --git a/packages/data/src/hooks/useFileWatch.test.ts b/packages/data/src/hooks/useFileWatch.test.ts index cc762e1e8..ae9a046c1 100644 --- a/packages/data/src/hooks/useFileWatch.test.ts +++ b/packages/data/src/hooks/useFileWatch.test.ts @@ -81,6 +81,21 @@ describe('useFileWatch', () => { expect(stateValues[2]).toBe(false); }); + it('resets stale state when starting a new watcher', () => { + stateValues = [ + { eventType: 'change', filename: 'old.txt' }, + new Error('old error'), + false, + ]; + + useFileWatch('next-path'); + effectCb?.(); + + expect(stateValues[0]).toBeNull(); + expect(stateValues[1]).toBeNull(); + expect(stateValues[2]).toBe(true); + }); + it('cleanup runs on unmount', () => { useFileWatch('test-path'); diff --git a/packages/data/src/hooks/useFileWatch.ts b/packages/data/src/hooks/useFileWatch.ts index 070ae2ecc..5f160e717 100644 --- a/packages/data/src/hooks/useFileWatch.ts +++ b/packages/data/src/hooks/useFileWatch.ts @@ -37,6 +37,9 @@ export function useFileWatch( useEffect(() => { let isMounted = true; let watcher: ReturnType | null = null; + setData(null); + setError(null); + setLoading(true); try { watcher = watch(path, { diff --git a/packages/data/src/hooks/useMutation.test.ts b/packages/data/src/hooks/useMutation.test.ts index 758f99066..64f5d4739 100644 --- a/packages/data/src/hooks/useMutation.test.ts +++ b/packages/data/src/hooks/useMutation.test.ts @@ -79,6 +79,63 @@ describe('useMutation', () => { expect(returnData).toEqual(mockResponseData); }); + it('treats successful empty responses as completed mutations', async () => { + mockFetch.mockResolvedValue({ + ok: true, + status: 204, + text: async () => '', + }); + + render(createElement(TestComponent, { url: '/api/test', method: 'DELETE' })); + + const returnData = await (global as any).hookResult.mutate(undefined); + await flushPromises(); + + const result = (global as any).hookResult; + expect(returnData).toBeNull(); + expect(result.loading).toBe(false); + expect(result.data).toBeNull(); + expect(result.error).toBeNull(); + expect(result.mutationCount).toBe(1); + }); + + it('does not let older concurrent mutations overwrite newer state', async () => { + let resolveFirst!: (response: any) => void; + let resolveSecond!: (response: any) => void; + + mockFetch + .mockImplementationOnce(() => new Promise(resolve => { resolveFirst = resolve; })) + .mockImplementationOnce(() => new Promise(resolve => { resolveSecond = resolve; })); + + render(createElement(TestComponent, { url: '/api/test' })); + + const mutate = (global as any).hookResult.mutate; + const first = mutate({ value: 'old' }); + const second = mutate({ value: 'new' }); + + resolveSecond({ + ok: true, + status: 200, + text: async () => JSON.stringify({ value: 'new' }), + }); + await second; + await flushPromises(); + expect((global as any).hookResult.data).toEqual({ value: 'new' }); + + resolveFirst({ + ok: true, + status: 200, + text: async () => JSON.stringify({ value: 'old' }), + }); + await first; + await flushPromises(); + + const result = (global as any).hookResult; + expect(result.data).toEqual({ value: 'new' }); + expect(result.mutationCount).toBe(1); + expect(result.loading).toBe(false); + }); + it('Error Handling on failed HTTP status', async () => { // Setup the mock fetch to simulate a 404 error mockFetch.mockResolvedValue({ @@ -127,4 +184,4 @@ describe('useMutation', () => { expect(result.data).toBeNull(); expect(result.error).toBe(networkError); }); -}); \ No newline at end of file +}); diff --git a/packages/data/src/hooks/useMutation.ts b/packages/data/src/hooks/useMutation.ts index 24b017754..63251e36a 100644 --- a/packages/data/src/hooks/useMutation.ts +++ b/packages/data/src/hooks/useMutation.ts @@ -1,11 +1,11 @@ // Mutation -import { useCallback, useState } from "@termuijs/jsx"; +import { useCallback, useRef, useState } from "@termuijs/jsx"; export type HttpMethod = 'POST' | 'PUT' | 'PATCH' | 'DELETE' export interface UseMutationReturn { - mutate: (payload: unknown) => Promise; + mutate: (payload: unknown) => Promise; reset: () => void; data: T | null; error: Error | null; @@ -13,8 +13,22 @@ export interface UseMutationReturn { mutationCount: number; } +async function readMutationResponse(response: Response): Promise { + if (response.status === 204 || response.status === 205) { + return null; + } + + if (typeof response.text === 'function') { + const text = await response.text(); + if (text.trim() === '') return null; + return JSON.parse(text) as T; + } + + return await response.json() as T; +} + /** - * useMutation — reactive HTTP mutation hook with loading and error states. + * useMutation - reactive HTTP mutation hook with loading and error states. * * Returns a `mutate` function that sends a request to the provided `url` * with the specified HTTP `method` (default: POST). Updates are tracked via @@ -28,8 +42,10 @@ export function useMutation(url: string, method: HttpMethod = 'POST const [error, setError] = useState(null) const [data, setData] = useState(null) const [mutationCount, setMutationCount] = useState(0) + const requestIdRef = useRef(0) - const mutate = useCallback(async (payload: unknown): Promise => { + const mutate = useCallback(async (payload: unknown): Promise => { + const requestId = ++requestIdRef.current setLoading(true) setError(null) @@ -46,17 +62,23 @@ export function useMutation(url: string, method: HttpMethod = 'POST throw new Error(`HTTP error! status: ${response.status}`) } - const result = (await response.json()) as T - setData(result) - setMutationCount((c)=> c+1) + const result = await readMutationResponse(response) + if (requestId === requestIdRef.current) { + setData(result) + setMutationCount((c)=> c+1) + } return result; } catch (err) { const errorObj = err instanceof Error ? err : new Error('Mutation failed'); - setError(errorObj); + if (requestId === requestIdRef.current) { + setError(errorObj); + } throw errorObj; } finally { - setLoading(false) + if (requestId === requestIdRef.current) { + setLoading(false) + } } }, [url, method]) @@ -68,4 +90,4 @@ export function useMutation(url: string, method: HttpMethod = 'POST }, []) return { mutate,reset, data, error, loading,mutationCount }; -} \ No newline at end of file +} diff --git a/packages/data/src/hooks/usePolling.test.ts b/packages/data/src/hooks/usePolling.test.ts index 54ce80add..330f24504 100644 --- a/packages/data/src/hooks/usePolling.test.ts +++ b/packages/data/src/hooks/usePolling.test.ts @@ -100,6 +100,23 @@ describe('usePolling', () => { expect((global as any).hookResult.data).toBe(3); }); + it('does not run immediately on dependency change while paused', async () => { + const fn = vi.fn().mockResolvedValue('data'); + const props = { fn, interval: 1000, deps: [1] as unknown[] }; + const { rerender } = render(createElement(TestComponent, props)); + + await flushPromises(); + expect(fn).toHaveBeenCalledTimes(1); + + (global as any).hookResult.pause(); + props.deps = [2]; + rerender(); + await flushPromises(); + + expect(fn).toHaveBeenCalledTimes(1); + expect((global as any).hookResult.paused).toBe(true); + }); + it('Skips interval tick while a request is still in flight', async () => { let resolveFirst!: (value: string) => void; const fn = vi.fn().mockImplementationOnce(() => new Promise(resolve => { diff --git a/packages/data/src/hooks/usePolling.ts b/packages/data/src/hooks/usePolling.ts index d34c973b2..902dd127d 100644 --- a/packages/data/src/hooks/usePolling.ts +++ b/packages/data/src/hooks/usePolling.ts @@ -39,8 +39,11 @@ export function usePolling( const requestIdRef = useRef(0); const adaptiveRef = useRef(null); - const execute = async () => { + const execute = async (force = false) => { const startedAt = Date.now(); + if (!force && pausedRef.current) { + return; + } if (inFlightRef.current) { adaptiveRef.current?.begin(); return; @@ -89,15 +92,17 @@ export function usePolling( }; const refresh = () => { - execute(); + execute(true); }; useEffect(() => { mountedRef.current = true; - setLoading(true); adaptiveRef.current = options.adaptive ? new AdaptivePollingController(options.adaptive) : null; - execute(); + if (!paused) { + setLoading(true); + execute(); + } if (adaptiveRef.current) { let timeout: ReturnType | undefined; @@ -131,7 +136,7 @@ export function usePolling( clearInterval(timer); }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [interval, options.adaptive, ...deps]); + }, [interval, options.adaptive, paused, ...deps]); return { data, error, loading, paused, pause, resume, refresh }; } diff --git a/packages/data/src/hooks/useSSE.test.ts b/packages/data/src/hooks/useSSE.test.ts index 0af257fd3..d22b9dfec 100644 --- a/packages/data/src/hooks/useSSE.test.ts +++ b/packages/data/src/hooks/useSSE.test.ts @@ -3,6 +3,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; let stateValues: unknown[] = []; let stateSetters: Array> = []; let effectCb: (() => (() => void) | void) | null = null; +let effectDeps: unknown[] | undefined; let stateCallCount = 0; vi.mock('@termuijs/jsx', () => ({ @@ -21,8 +22,9 @@ vi.mock('@termuijs/jsx', () => ({ } return [stateValues[id], stateSetters[id]]; }, - useEffect: (cb: () => (() => void) | void) => { + useEffect: (cb: () => (() => void) | void, deps?: unknown[]) => { effectCb = cb; + effectDeps = deps; }, useInterval: vi.fn(), })); @@ -64,6 +66,7 @@ describe('useSSE', () => { stateSetters = []; stateCallCount = 0; effectCb = null; + effectDeps = undefined; activeSources = []; vi.stubGlobal('EventSource', MockEventSource); }); @@ -98,6 +101,24 @@ describe('useSSE', () => { expect(stateValues[2]).toBe(false); }); + it('reruns the effect when the parser callback changes', () => { + const parse = (raw: string) => ({ value: raw }); + useSSE('https://example.com/events', parse); + + expect(effectDeps).toEqual(['https://example.com/events', parse]); + }); + + it('resets stale state when a subscription starts', () => { + stateValues = ['old-event', new Error('old error'), false]; + + useSSE('https://example.com/events'); + effectCb?.(); + + expect(stateValues[0]).toBeNull(); + expect(stateValues[1]).toBeNull(); + expect(stateValues[2]).toBe(true); + }); + it('cleanup runs on unmount', () => { useSSE('https://example.com/events'); diff --git a/packages/data/src/hooks/useSSE.ts b/packages/data/src/hooks/useSSE.ts index 1390f80d9..3a9094dd8 100644 --- a/packages/data/src/hooks/useSSE.ts +++ b/packages/data/src/hooks/useSSE.ts @@ -28,6 +28,9 @@ export function useSSE( useEffect(() => { let isMounted = true; + setData(null); + setError(null); + setLoading(true); if (typeof EventSource === 'undefined') { setError(new Error('EventSource is not supported in this environment')); @@ -61,7 +64,7 @@ export function useSSE( isMounted = false; source.close(); }; - }, [url]); + }, [url, parse]); return { data, error, loading }; } diff --git a/packages/data/src/http.test.ts b/packages/data/src/http.test.ts index 31eea87ad..cd1d8b285 100644 --- a/packages/data/src/http.test.ts +++ b/packages/data/src/http.test.ts @@ -80,6 +80,8 @@ describe('http data provider', () => { latency: 0, statusCode: 0, }); + + expect(http.latency('http://error.com')).toEqual([0]); }); it('stores rolling history and caps it at MAX_HISTORY (100)', async () => { diff --git a/packages/data/src/http.ts b/packages/data/src/http.ts index b313ac528..7ffeb0021 100644 --- a/packages/data/src/http.ts +++ b/packages/data/src/http.ts @@ -19,6 +19,19 @@ const _latencyHistory = new Map(); const MAX_HISTORY = 100; const MAX_URLS = 100; +function recordLatency(url: string, latency: number): void { + if (!_latencyHistory.has(url)) { + if (_latencyHistory.size >= MAX_URLS) { + const oldest = _latencyHistory.keys().next().value; + if (oldest !== undefined) _latencyHistory.delete(oldest); + } + _latencyHistory.set(url, []); + } + const history = _latencyHistory.get(url)!; + history.push(latency); + if (history.length > MAX_HISTORY) history.shift(); +} + /** HTTP data provider — uses native fetch (Node 18+) */ export const http = { /** @@ -41,18 +54,7 @@ export const http = { // Consume response body to prevent connection leaks await res.text().catch(() => { }); const latency = Date.now() - start; - - // Store latency history - if (!_latencyHistory.has(url)) { - if (_latencyHistory.size >= MAX_URLS) { - const oldest = _latencyHistory.keys().next().value; - if (oldest !== undefined) _latencyHistory.delete(oldest); - } - _latencyHistory.set(url, []); - } - const history = _latencyHistory.get(url)!; - history.push(latency); - if (history.length > MAX_HISTORY) history.shift(); + recordLatency(url, latency); return { name: url, @@ -63,6 +65,7 @@ export const http = { }; } catch { const latency = Date.now() - start; + recordLatency(url, latency); return { name: url, url, status: 'down', latency, statusCode: 0 }; } }, diff --git a/packages/data/src/useWebSocket.test.tsx b/packages/data/src/useWebSocket.test.tsx index 06fbad0ef..545c573df 100644 --- a/packages/data/src/useWebSocket.test.tsx +++ b/packages/data/src/useWebSocket.test.tsx @@ -138,6 +138,20 @@ describe('useWebSocket hook', () => { expect(activeSockets[2].url).toBe('wss://new.com') }) + it('clears the previous message when the url changes', async () => { + const { rerender } = render() + activeSockets[0].onopen?.() + activeSockets[0].onmessage?.({ data: 'old message' }) + await Promise.resolve() + expect((global as any).hookResult.message).toBe('old message') + + rerender() + await Promise.resolve() + + expect((global as any).hookResult.message).toBeNull() + expect((global as any).hookResult.state).toBe('connecting') + }) + it('rapid url changes do not accumulate reconnect timers', () => { const { rerender } = render() // Simulate disconnect which schedules reconnect @@ -179,4 +193,4 @@ describe('useWebSocket hook', () => { vi.advanceTimersByTime(1000) expect(activeSockets.length).toBe(2) }) -}) \ No newline at end of file +})