Fix retry predicate's dead instanceof Error check for 404s

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: {

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<ApiError>(
+    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"', () => {

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<T,
+ * Error>` 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<string, string[]>
+
+  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<ApiError>): 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<ApiError>) => {
-      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<ApiError>) => Promise.reject(normalizeApiError(error)),
   )
 
   return client

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)
+  })
+})

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<T,
+ * Error>` 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
+}