From f13ee1369b0f7a1b7deafbf641c456803264a334 Mon Sep 17 00:00:00 2001 From: richardtoms100 <315124855+richardtoms100@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:22:18 +0100 Subject: [PATCH 1/4] Reject API errors with a real Error subclass instead of a plain object --- src/lib/api.ts | 59 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 12 deletions(-) diff --git a/src/lib/api.ts b/src/lib/api.ts index 0811bc0..e1e5312 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -24,6 +24,52 @@ import type { const BASE_URL = (import.meta.env.VITE_API_URL as string) || 'http://localhost:8080' +// ─── Normalized API errors ──────────────────────────────────────────────────── + +/** + * A real `Error` subclass carrying the same `code`/`details` fields as + * `ApiError`, rejected by every request through `apiClient` in place of a + * plain `{ code, message, details }` object literal. The plain-object + * rejection made `error instanceof Error` — used by the global React Query + * retry predicate in `App.tsx` (and assumed by every hook's `useQuery` type parameter) — always false for backend-routed failures, + * silently defeating the "don't retry 404s" check (#60). + */ +export class ApiRequestError extends Error implements ApiError { + code: string + details?: Record + + constructor(apiError: ApiError) { + super(apiError.message) + this.name = 'ApiRequestError' + this.code = apiError.code + this.details = apiError.details + } +} + +/** + * Turns a rejected axios response into an `ApiRequestError`. Exported + * separately from the interceptor so it can be unit-tested directly instead + * of relying on axios's internal interceptor-handler storage. + * + * `code` prefers the backend's own `data.code`; when the backend doesn't + * send one, a 404 status is normalized to the stable `'NOT_FOUND'` code + * (HTTP status is a reliable, backend-convention-independent signal, unlike + * substring-matching `message`) rather than falling through to + * `'UNKNOWN_ERROR'`. + */ +export function normalizeApiError(error: AxiosError): ApiRequestError { + const message = + error.response?.data?.message || error.message || 'An unexpected error occurred' + const code = + error.response?.data?.code || (error.response?.status === 404 ? 'NOT_FOUND' : 'UNKNOWN_ERROR') + return new ApiRequestError({ + code, + message, + details: error.response?.data?.details, + }) +} + // ─── Axios instance ─────────────────────────────────────────────────────────── const createApiClient = (): AxiosInstance => { @@ -51,18 +97,7 @@ const createApiClient = (): AxiosInstance => { // Response interceptor — normalise errors client.interceptors.response.use( (response) => response, - (error: AxiosError) => { - const message = - error.response?.data?.message || - error.message || - 'An unexpected error occurred' - const apiError: ApiError = { - code: error.response?.data?.code || 'UNKNOWN_ERROR', - message, - details: error.response?.data?.details, - } - return Promise.reject(apiError) - }, + (error: AxiosError) => Promise.reject(normalizeApiError(error)), ) return client From ac6696c1bc8b7417d20a08d83971089ce2ceb9d0 Mon Sep 17 00:00:00 2001 From: richardtoms100 <315124855+richardtoms100@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:22:18 +0100 Subject: [PATCH 2/4] Extract the retry predicate and use ApiRequestError's code to skip 404 retries --- src/App.tsx | 7 ++----- src/lib/queryRetry.ts | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) create mode 100644 src/lib/queryRetry.ts diff --git a/src/App.tsx b/src/App.tsx index a6bf03b..4971fb8 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -4,6 +4,7 @@ import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import { ReactQueryDevtools } from '@tanstack/react-query-devtools' import { WalletProvider } from '@/context/WalletContext' import { Layout } from '@/components/layout/Layout' +import { shouldRetryQuery } from '@/lib/queryRetry' // Pages (lazy-loaded for code-splitting) import { lazy, Suspense } from 'react' @@ -28,11 +29,7 @@ const queryClient = new QueryClient({ queries: { staleTime: 30_000, gcTime: 5 * 60_000, - retry: (failureCount, error) => { - // Don't retry 404s - if (error instanceof Error && error.message.includes('not found')) return false - return failureCount < 2 - }, + retry: shouldRetryQuery, refetchOnWindowFocus: true, }, mutations: { diff --git a/src/lib/queryRetry.ts b/src/lib/queryRetry.ts new file mode 100644 index 0000000..d7f5235 --- /dev/null +++ b/src/lib/queryRetry.ts @@ -0,0 +1,26 @@ +import { ApiRequestError } from '@/lib/api' + +/** + * The global default React Query `retry` predicate: don't retry 404s. + * `apiClient` rejects with a real `ApiRequestError` (not a plain object) so + * this check — and the generic `Error` typing every hook's `useQuery` already assumed — actually matches (#60). `code === 'NOT_FOUND'` + * is the primary signal (derived from the real HTTP 404 status, not + * backend-specific conventions); the message check is kept as a fallback + * for whatever a specific backend error code might otherwise indicate. + * + * Lives in its own module (rather than inline in App.tsx's `queryClient` + * config) both so it's directly unit-testable per #60's own suggested + * testing strategy, and so App.tsx — a component file — only exports a + * component, which `react-refresh/only-export-components` requires for Fast + * Refresh to work correctly. + */ +export function shouldRetryQuery(failureCount: number, error: unknown): boolean { + if ( + error instanceof ApiRequestError && + (error.code === 'NOT_FOUND' || error.message.includes('not found')) + ) { + return false + } + return failureCount < 2 +} From ec0db44fcb0e59e37e308464779bbc13dd3cfc24 Mon Sep 17 00:00:00 2001 From: richardtoms100 <315124855+richardtoms100@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:22:18 +0100 Subject: [PATCH 3/4] Add tests for normalizeApiError and ApiRequestError --- src/lib/api.test.ts | 71 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index bd1e5fb..08d6ec7 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -1,5 +1,74 @@ import { describe, it, expect, vi, afterEach } from 'vitest' -import { horizonUrl, fetchAccountFromHorizon } from './api' +import { AxiosError, AxiosHeaders } from 'axios' +import { horizonUrl, fetchAccountFromHorizon, normalizeApiError, ApiRequestError } from './api' +import type { ApiError } from '@/types' + +function makeAxiosError(status: number | undefined, data?: ApiError, message = 'Request failed') { + return new AxiosError( + message, + undefined, + { headers: new AxiosHeaders() }, + undefined, + status === undefined + ? undefined + : { + status, + statusText: '', + headers: {}, + config: { headers: new AxiosHeaders() }, + data: data as ApiError, + }, + ) +} + +describe('normalizeApiError', () => { + it('produces a real Error instance, not a plain object (#60)', () => { + const result = normalizeApiError(makeAxiosError(404, undefined)) + + expect(result).toBeInstanceOf(Error) + expect(result).toBeInstanceOf(ApiRequestError) + }) + + it('carries the backend-provided code, message, and details through unchanged', () => { + const result = normalizeApiError( + makeAxiosError(400, { + code: 'VALIDATION_ERROR', + message: 'Amount must be positive', + details: { amount: ['must be greater than 0'] }, + }), + ) + + expect(result.code).toBe('VALIDATION_ERROR') + expect(result.message).toBe('Amount must be positive') + expect(result.details).toEqual({ amount: ['must be greater than 0'] }) + }) + + it("derives code 'NOT_FOUND' from a 404 status when the backend sends no code", () => { + const result = normalizeApiError(makeAxiosError(404, undefined, 'Request failed with status code 404')) + + expect(result.code).toBe('NOT_FOUND') + }) + + it('prefers a backend-supplied code over the derived NOT_FOUND for a 404', () => { + const result = normalizeApiError( + makeAxiosError(404, { code: 'PAYMENT_REQUEST_NOT_FOUND', message: 'Payment request not found' }), + ) + + expect(result.code).toBe('PAYMENT_REQUEST_NOT_FOUND') + }) + + it("falls back to 'UNKNOWN_ERROR' for a non-404 failure with no backend code", () => { + const result = normalizeApiError(makeAxiosError(500, undefined, 'Request failed with status code 500')) + + expect(result.code).toBe('UNKNOWN_ERROR') + }) + + it('falls back to a generic message when neither the backend nor axios supplies one', () => { + const result = normalizeApiError(makeAxiosError(undefined, undefined, '')) + + expect(result.message).toBe('An unexpected error occurred') + }) +}) describe('horizonUrl', () => { it('returns the testnet Horizon host for network "testnet"', () => { From 061a9dbb901d76b8d43378105fcb652c6e0c64b0 Mon Sep 17 00:00:00 2001 From: richardtoms100 <315124855+richardtoms100@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:22:18 +0100 Subject: [PATCH 4/4] Add tests for shouldRetryQuery's 404 handling --- src/lib/queryRetry.test.ts | 48 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 src/lib/queryRetry.test.ts diff --git a/src/lib/queryRetry.test.ts b/src/lib/queryRetry.test.ts new file mode 100644 index 0000000..84c9010 --- /dev/null +++ b/src/lib/queryRetry.test.ts @@ -0,0 +1,48 @@ +import { describe, it, expect } from 'vitest' +import { shouldRetryQuery } from './queryRetry' +import { ApiRequestError } from '@/lib/api' + +// #60: the retry predicate previously checked `error instanceof Error`, but +// apiClient rejected with a plain `{ code, message, details }` object +// literal — never a real Error — so the "don't retry 404s" branch was +// unreachable for every backend-routed query relying on the global default. +// These tests exercise `shouldRetryQuery` directly, exactly as it's wired +// into `queryClient`'s `defaultOptions.queries.retry` in App.tsx. +describe('shouldRetryQuery', () => { + it("does not retry when the error is an ApiRequestError with code 'NOT_FOUND'", () => { + const error = new ApiRequestError({ code: 'NOT_FOUND', message: 'Escrow not found' }) + + expect(shouldRetryQuery(0, error)).toBe(false) + }) + + it('does not retry an ApiRequestError whose message mentions "not found", even with a different code', () => { + const error = new ApiRequestError({ + code: 'PAYMENT_REQUEST_NOT_FOUND', + message: 'Payment request not found', + }) + + expect(shouldRetryQuery(0, error)).toBe(false) + }) + + it('retries an ApiRequestError for a different, non-404 failure up to the failure-count limit', () => { + const error = new ApiRequestError({ code: 'UNKNOWN_ERROR', message: 'Internal server error' }) + + expect(shouldRetryQuery(0, error)).toBe(true) + expect(shouldRetryQuery(1, error)).toBe(true) + expect(shouldRetryQuery(2, error)).toBe(false) + }) + + it('retries a plain, non-ApiRequestError value up to the failure-count limit', () => { + // A non-API error (e.g. a thrown string, or a plain object from code + // this predicate isn't meant to special-case) must never be mistaken + // for a 404 just because it happens to be object-shaped. + expect(shouldRetryQuery(0, { code: 'NOT_FOUND', message: 'not found' })).toBe(true) + expect(shouldRetryQuery(2, { code: 'NOT_FOUND', message: 'not found' })).toBe(false) + }) + + it('retries a bare Error instance (not an ApiRequestError) up to the failure-count limit', () => { + const error = new Error('not found') + + expect(shouldRetryQuery(0, error)).toBe(true) + }) +})