Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a unified API client layer to replace direct axiosInstance usage across the codebase, introducing centralized error handling, request/response interceptors, and automatic retry logic for transient failures.
Key changes:
- New
apiClientwrapper with type-safe responses and automatic error transformation - Centralized error handling with user-friendly Spanish messages and retry logic for network/server errors
- Request/response interceptors for logging, authentication token injection, and token refresh
- Comprehensive test suite for API client and error handler functionality
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/types/api.ts |
Defines TypeScript interfaces for API responses, request configuration, retry logic, and request logging |
src/lib/errorHandler.ts |
Implements centralized error processing for Axios errors with Spanish error messages and retry eligibility checks |
src/lib/apiInterceptors.ts |
Configures global Axios interceptors for request logging, auth token injection, and automatic token refresh on 401 errors |
src/lib/apiClient.ts |
Main API client class providing type-safe HTTP methods (GET, POST, PUT, DELETE, PATCH) with automatic retry and error handling |
src/services/user.ts |
Migrated from axiosInstance to apiClient for user update and delete operations |
src/services/stopMonitoring.ts |
Refactored to use apiClient with consistent error handling |
src/services/startMonitoring.ts |
Refactored to use apiClient with consistent error handling |
src/services/signUp.ts |
Updated to use apiClient, improved formatting, maintains redirect behavior on success |
src/services/login.ts |
Migrated to apiClient with typed LoginResponse interface |
src/services/getAuth.ts |
Updated to use apiClient with typed UserData response and improved error logging |
src/services/fuente.ts |
Migrated all CRUD and monitoring operations to apiClient |
src/services/eje.ts |
Refactored all operations to use apiClient with consistent error handling |
src/services/boletines.ts |
Updated FormData handling to use apiClient.postFormData method |
src/__tests__/lib/apiClient.test.ts |
Comprehensive test suite covering HTTP methods, error handling, and retry logic helpers |
docs/TASK-*.MD |
Task documentation describing the implementation approach and requirements |
docs/SCHEMA.md |
Schema documentation for data models and validation |
docs/MODULES.md |
Module integration guide with code templates and best practices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Auto-inicializar interceptores | ||
| initializeInterceptors(); | ||
|
|
There was a problem hiding this comment.
The interceptors are never initialized because this file is not imported anywhere in the codebase. The auto-initialization on line 213 will only run when the module is imported. Consider either: 1) importing this file in a main entry point (e.g., a root layout or app initialization file), or 2) importing and calling initializeInterceptors in src/lib/apiClient.ts so it's initialized when the apiClient is first used.
| // Auto-inicializar interceptores | |
| initializeInterceptors(); |
| // Agregar token de autenticación si existe (solo en cliente) | ||
| if (typeof window !== 'undefined') { | ||
| const token = getAuthToken(); | ||
| if (token && !config.headers.Authorization) { | ||
| config.headers.Authorization = `Bearer ${token}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
The request interceptor only adds the Authorization header when running in the browser (typeof window !== 'undefined'), but many services in this PR are server actions that run on the server. This means server-side API calls won't automatically include the auth token from cookies. Server actions like getAuth manually add the Authorization header, but other server actions may need authentication. Consider either: 1) checking for server-side cookies in the interceptor when running on the server, or 2) documenting that server actions must manually add auth headers when needed.
| // Agregar providers necesarios (theme, context, etc.) | ||
| }; | ||
|
|
||
| export const waitForLoadingToFinish = () => screen.findByText(/loading/i, {}, { timeout: 3000 }); |
There was a problem hiding this comment.
Typo: "screen.findByText" should be imported from '@testing-library/react' but 'screen' is not imported in the example code. The line appears incomplete or missing the import statement.
| async postFormData<T>( | ||
| path: string, | ||
| formData: FormData, | ||
| config?: RequestConfig | ||
| ): Promise<ApiResponse<T>> { | ||
| try { | ||
| const { data } = await axiosInstance.post<T>(path, formData, { | ||
| headers: { | ||
| 'Content-Type': 'multipart/form-data', | ||
| ...config?.headers, | ||
| }, | ||
| params: config?.params, | ||
| timeout: config?.timeout, | ||
| }); | ||
|
|
||
| return { | ||
| success: true, | ||
| data, | ||
| }; | ||
| } catch (error) { | ||
| const apiError = handleApiError(error, path); | ||
| return { | ||
| success: false, | ||
| error: apiError.message, | ||
| code: apiError.code, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The postFormData and putFormData methods don't implement retry logic, unlike the regular request methods which use the private request() method with retry capabilities. This creates inconsistent behavior where FormData uploads won't be retried on transient failures (network errors, 5xx errors). Consider refactoring to use the same retry logic or documenting why FormData uploads shouldn't be retried.
| // Manejar errores de autenticación (401) | ||
| if (error.response?.status === 401 && typeof window !== 'undefined') { | ||
| const handled = await handleUnauthorizedError(); | ||
| if (handled && config) { | ||
| // Reintentar la petición original con nuevo token | ||
| const newToken = getAuthToken(); | ||
| if (newToken && config.headers) { | ||
| config.headers.Authorization = `Bearer ${newToken}`; | ||
| return axiosInstance.request(config); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The refresh token handling logic attempts to refresh the token and retry the request, but this creates a potential issue: if the refresh also returns 401 (e.g., refresh token is also expired), this could cause an infinite loop. The handleUnauthorizedError function doesn't prevent retrying the same request multiple times. Consider adding a flag to prevent infinite retry loops or tracking retry attempts for 401 errors.
| if (Array.isArray(data[field]) && data[field].length > 0) { | ||
| const firstError = data[field][0]; | ||
| if (typeof firstError === 'string') { | ||
| return firstError; | ||
| } | ||
| if (typeof firstError === 'object' && firstError !== null) { | ||
| return (firstError as Record<string, string>).message || | ||
| (firstError as Record<string, string>).msg || | ||
| null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential type safety issue with array access. The code accesses data[field] which is typed as unknown, but then directly accesses data[field].length and data[field][0] without proper type narrowing. TypeScript should be able to infer the type after the Array.isArray check, but the explicit cast or proper type assertion would be safer. Consider: const fieldValue = data[field]; if (Array.isArray(fieldValue) && fieldValue.length > 0) { const firstError = fieldValue[0]; ... }
| if ( | ||
| attemptNumber < this.retryConfig.maxRetries && | ||
| isRetryableError(error) | ||
| ) { | ||
| const delay = getRetryDelay(attemptNumber, this.retryConfig.retryDelay); | ||
|
|
||
| if (typeof process !== 'undefined' && process.env.NODE_ENV === 'development') { | ||
| console.log( | ||
| `[API Retry] ${method} ${path} - Attempt ${attemptNumber + 1}/${this.retryConfig.maxRetries} after ${delay}ms` | ||
| ); | ||
| } | ||
|
|
||
| await this.sleep(delay); | ||
| return this.request<T>(method, path, body, config, attemptNumber + 1); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the retry logic in apiClient. While isRetryableError and getRetryDelay are tested, there are no tests verifying that apiClient.get/post/put/delete actually retry requests when they fail with retryable errors. Consider adding integration tests that verify retry behavior, such as testing that a 500 error triggers retries up to maxRetries times.
| function getAuthToken(): string | null { | ||
| if (typeof window === 'undefined') return null; | ||
|
|
||
| // Intentar obtener token de cookies usando js-cookie si está disponible | ||
| try { | ||
| const cookies = document.cookie.split(';'); | ||
| for (const cookie of cookies) { | ||
| const [name, value] = cookie.trim().split('='); | ||
| if (name === 'access_token') { | ||
| return value; | ||
| } | ||
| } | ||
| } catch { | ||
| return null; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Obtiene el refresh token de las cookies | ||
| */ | ||
| function getRefreshToken(): string | null { | ||
| if (typeof window === 'undefined') return null; | ||
|
|
||
| try { | ||
| const cookies = document.cookie.split(';'); | ||
| for (const cookie of cookies) { | ||
| const [name, value] = cookie.trim().split('='); | ||
| if (name === 'refresh_token') { | ||
| return value; | ||
| } | ||
| } | ||
| } catch { | ||
| return null; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Maneja errores de autenticación intentando refrescar el token | ||
| */ | ||
| async function handleUnauthorizedError(): Promise<boolean> { | ||
| const refreshToken = getRefreshToken(); | ||
|
|
||
| if (!refreshToken) { | ||
| // No hay refresh token, redirigir a login | ||
| clearAuthTokens(); | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| // Intentar refrescar el token | ||
| const response = await axiosInstance.post('token/refresh/', { | ||
| refresh: refreshToken, | ||
| }); | ||
|
|
||
| if (response.data?.access) { | ||
| // Guardar nuevo access token | ||
| setAuthToken(response.data.access); | ||
| return true; | ||
| } | ||
| } catch { | ||
| // Falló el refresh, limpiar tokens | ||
| clearAuthTokens(); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Guarda el token de autenticación en cookies | ||
| */ | ||
| function setAuthToken(token: string): void { | ||
| if (typeof window === 'undefined') return; | ||
|
|
||
| // Configurar cookie con opciones de seguridad | ||
| const expires = new Date(); | ||
| expires.setDate(expires.getDate() + 1); // 1 día | ||
|
|
||
| document.cookie = `access_token=${token}; expires=${expires.toUTCString()}; path=/; secure; samesite=strict`; |
There was a problem hiding this comment.
Inconsistent cookie handling: the login service (src/services/login.ts) uses the js-cookie library to set cookies, which properly handles security attributes and provides a consistent API. However, this file manually manipulates cookies using document.cookie. Consider using js-cookie here as well for consistency and to ensure cookies are set correctly across the codebase. Reference: src/services/login.ts:19-28
No description provided.