fix: add apiClient with status validation to prevent caching of error…gssoc

[vaishnavi003-svg]

PR Description

Added a new apiClient.ts file that validates HTTP status codes before returning responses. Only successful (2xx) responses are now cached. Error responses (4xx, 5xx) throw errors and are not cached, preventing stale error data from being served to users.

Type of Change

• Bug fix (change which fixes an issue)
• New feature (change which adds functionality)
• Documentation update

Select your work-area

• Frontend
• Backend
• Documentation
• Others

Related Issue

#1197

Add your Work Example

📷 No UI changes - This is a backend logic fix. The fix ensures that when an API returns a 5xx error, that error response is NOT cached. Subsequent requests will hit the actual API instead of receiving a stale error.

Fixes (mention the issue number which this fixes)

Fixes #1197

Checklist

• I have updated my branch and synced it with the project's 'develop' branch before making this PR.
• I have optimized the file changes.
• I have added a snapshot of my work example.
• I have made a PR to the project's develop branch.

Undertaking

• My code follows the style guidelines of this project.

• I have performed a self-review of my code.

• I have commented my code, particularly in hard-to-understand areas.

• I have made corresponding changes to the documentation.

• I have checked for plagiarism and assure its authenticity.

• I have read and followed the code of conduct for this repository. I understand that violation of this undertaking may have legal consequences.

• I Agree

[github-actions]

Thank you @, for creating the PR and contributing to our UltimateHealth project 💗.
Our team will review the PR and will reach out to you soon! 😇
Make sure that you have marked all the tasks that you are done with ✅.
Thank you for your patience! 😀

[github-actions]

❌ PR Validation Failed

This PR was marked as gssoc:invalid because: No linked issues found in the PR description using standard keywords (e.g., 'Fixes #123').

To resolve this, please ensure your PR description links an issue (e.g., Fixes #123) and that you are either the creator or an assignee of that issue.

[SB2318]

❌ PR Validation Failed

This PR was marked as gssoc:invalid because: No linked issues found in the PR description using standard keywords (e.g., 'Fixes #123').

To resolve this, please ensure your PR description links an issue (e.g., Fixes #123) and that you are either the creator or an assignee of that issue.

I will kill you if you do not fix by tomorrow.

[vaishnavi003-svg]

@SB2318
Fixed: Changed #closes 1197 to Fixes #1197 in description.
thank you

[SB2318]

/review

[github-actions]

🤖 Gemini AI Code Review

Summary

This Pull Request aims to address issue #1197 by introducing a new apiClient.ts service. The primary goal is to prevent error responses (4xx, 5xx) from being cached by ensuring that only successful (2xx) HTTP responses are returned, while others throw an error. This is a crucial improvement for data integrity and user experience.

While the intent is excellent and the core idea of validating status codes is sound, the implementation introduces several critical issues, including syntax errors, runtime errors, and significant architectural changes unrelated to the stated purpose. The PR also contains inconsistencies in error handling and potentially breaks existing functionality due to unrelated changes.

🔴 High Severity

• Issue: PROD_URL Redefinition and Hardcoded Development URL
  The PROD_URL constant is declared twice, once with a hardcoded Android emulator localhost address and then immediately re-declared using Constants.expoConfig?.extra. This is a syntax error (SyntaxError: Identifier 'PROD_URL' has already been declared) and will prevent the application from compiling or running. Furthermore, a hardcoded development URL should never be present in production-bound code.

  • Impact: Application will fail to compile/run. If it somehow bypasses compilation, it would incorrectly point to a local development server in production environments.
  • Fix:

    // frontend/src/services/apiClient.ts
    import Constants from 'expo-constants'; // <--- Add this import
    
    // Use a single, consistent way to define PROD_URL
    // Prioritize the extra config for production, fallback to a sensible default for development
    const PROD_URL: string = Constants.expoConfig?.extra?.PROD_URL || "http://localhost:3000/api"; // Or a specific dev URL
    // Remove the duplicate declaration: const PROD_URL = "http://10.0.2.2:3000/api";

• Issue: Missing Constants Import
  The Constants object is used to access expoConfig?.extra but is not imported from expo-constants.

  • Impact: This will result in a runtime error (ReferenceError: Can't find variable: Constants) when the apiClient is initialized, crashing the application.
  • Fix:

    // frontend/src/services/apiClient.ts
    import Constants from 'expo-constants'; // <--- Add this line at the top of the file
    // ... rest of the file

• Issue: Inconsistent Error Handling and Object Types
  The apiRequest function throws two different types of errors:

  1. For HTTP errors (non-2xx), it throws a custom object: { success: false, status: response.status, error: errorData }.
  2. For network errors (in the catch block), it re-throws the raw error object, which is typically an instance of Error.
  • Impact: Consumers of apiClient will have to implement inconsistent error handling logic, checking for both custom object properties (error.success) and instanceof Error. This makes error handling brittle, prone to bugs, and harder to maintain. Standard Error objects also provide valuable stack traces, which are lost with custom plain objects.
  • Fix: Introduce a custom error class that extends Error for all API-related errors, ensuring consistency and preserving stack traces.

    // frontend/src/services/apiClient.ts
    import Constants from 'expo-constants';
    
    export class ApiError extends Error {
      status: number | undefined;
      data: any; // Or a more specific error data type
    
      constructor(message: string, status?: number, data?: any) {
        super(message);
        this.name = 'ApiError';
        this.status = status;
        this.data = data;
        // Set the prototype explicitly to ensure instanceof works correctly
        Object.setPrototypeOf(this, ApiError.prototype);
      }
    }
    
    const PROD_URL: string = Constants.expoConfig?.extra?.PROD_URL || "http://localhost:3000/api";
    
    export async function apiRequest(endpoint: string, options?: RequestInit) {
      const url = `${PROD_URL}${endpoint}`;
    
      try {
        const response = await fetch(url, {
          ...options,
          headers: {
            'Content-Type': 'application/json',
            ...options?.headers,
          },
        });
    
        if (response.ok) {
          const data = await response.json();
          return {
            success: true,
            data: data,
            status: response.status
          };
        } else {
          const errorData = await response.json().catch(() => ({}));
          throw new ApiError(
            `API request failed with status ${response.status}`,
            response.status,
            errorData
          );
        }
      } catch (error) {
        if (error instanceof ApiError) {
          throw error; // Re-throw our custom API error
        } else if (error instanceof Error) {
          // For network errors or other unexpected errors, wrap them in our custom error
          throw new ApiError(`Network or unexpected error: ${error.message}`, undefined, error);
        } else {
          // Fallback for truly unknown error types
          throw new ApiError(`An unknown error occurred: ${String(error)}`);
        }
      }
    }
    // ... rest of the file

• Issue: Unrelated Change: audio-module Removal in package.json
  The package.json diff shows the removal of "audio-module": "workspace:*". This change is completely unrelated to the PR's stated purpose of fixing API caching for error responses.

  • Impact: If audio-module was a dependency for other parts of the application, its removal could introduce new bugs or break existing features. Unrelated changes make PRs harder to review, increase the risk of regressions, and complicate rollbacks.
  • Fix: Revert this change. If audio-module is genuinely no longer needed, it should be removed in a separate, dedicated PR with proper justification and testing.

• Issue: Unrelated and Potentially Breaking Changes in pnpm-workspace.yaml
  The pnpm-workspace.yaml file is modified to add . (the current directory) and packages/* to the packages list. While adding services/* is relevant for the new apiClient.ts, adding . and packages/* significantly alters how pnpm manages the workspace.

  • Impact: This change could have wide-ranging, unintended consequences for dependency resolution, build processes, and how other packages in the monorepo interact. It might lead to unexpected dependency hoisting, duplicate installations, or issues with tooling that expects a different workspace structure. This is a major architectural change that needs explicit justification and thorough testing, which is outside the scope of this bug fix PR.
  • Fix: Revert the additions of . and packages/*. Only add services/* if it's truly necessary for pnpm to manage frontend/services as a separate workspace package (which is usually not the case for a simple directory of utility files). If frontend/services is not intended to be a standalone package, then services/* should also be removed.

🟡 Medium Severity

• Issue: Default Content-Type: 'application/json' Header
  The apiRequest function unconditionally sets 'Content-Type': 'application/json' in the headers. While common for JSON-based APIs, this can be problematic for endpoints that expect other content types (e.g., multipart/form-data for file uploads, application/x-www-form-urlencoded).

  • Impact: Future API calls requiring different content types will fail or behave unexpectedly unless the header is explicitly overridden in every call, which is error-prone.
  • Fix: Make the Content-Type header more flexible. It should either be set only when body is an object (and JSON.stringify is used), or it should be easily overridable without merging issues.

    // frontend/src/services/apiClient.ts
    // ...
    export async function apiRequest(endpoint: string, options?: RequestInit) {
      const url = `${PROD_URL}${endpoint}`;
    
      const defaultHeaders = {
        // Only set Content-Type to application/json if a body is provided and it's an object
        // Otherwise, let the browser/fetch API determine it or allow explicit override
        ...(options?.body && typeof options.body === 'string' && options.headers?.['Content-Type'] === undefined
            ? { 'Content-Type': 'application/json' }
            : {}),
        ...options?.headers,
      };
    
      try {
        const response = await fetch(url, {
          ...options,
          headers: defaultHeaders,
        });
        // ...
      }
    }
    
    // For POST/PUT, ensure JSON.stringify is used and Content-Type is set if not already
    export async function post(endpoint: string, data: any) {
      return apiRequest(endpoint, {
        method: 'POST',
        body: JSON.stringify(data),
        headers: {
          'Content-Type': 'application/json' // Explicitly set for JSON bodies
        }
      });
    }
    // Apply similar logic to put

• Issue: Ambiguous "Caching" Description
  The PR description states: "Only successful (2xx) responses are now cached. Error responses (4xx, 5xx) throw errors and are not cached". The apiClient itself does not implement a caching mechanism. Instead, by throwing errors for non-2xx responses, it prevents downstream caching libraries (like @tanstack/react-query or browser caches that respect HTTP status codes) from caching error states.

  • Impact: While the outcome is desired, the description might mislead readers into thinking the apiClient itself has caching logic. This could lead to confusion or incorrect assumptions about the system's caching behavior.
  • Fix: Clarify the description to accurately reflect that the apiClient facilitates preventing error caching by throwing errors, rather than implementing caching itself. E.g., "Error responses (4xx, 5xx) now throw errors, ensuring that downstream caching mechanisms do not store stale error data."

• Issue: Use of any Type for Request Data
  The post and put helper functions use data: any for the request body.

  • Impact: Using any bypasses TypeScript's type checking, removing compile-time safety and making it easier to pass incorrect data structures to API endpoints.
  • Fix: Use unknown instead of any to force type checking at the point of use, or define specific interfaces for common request bodies.

    // frontend/src/services/apiClient.ts
    // ...
    export async function post(endpoint: string, data: unknown) { // Use unknown
      return apiRequest(endpoint, {
        method: 'POST',
        body: JSON.stringify(data),
        headers: { 'Content-Type': 'application/json' }
      });
    }
    
    export async function put(endpoint: string, data: unknown) { // Use unknown
      return apiRequest(endpoint, {
        method: 'PUT',
        body: JSON.stringify(data),
        headers: { 'Content-Type': 'application/json' }
      });
    }

🟢 Low Severity / Nits

• Issue: Unprofessional Comment
  The comment // 🔥 Main fix: Sirf successful responses return contains an informal emoji and a Hindi word ("Sirf" meaning "only").

  • Fix: Rephrase to be professional and clear in English: // Only successful (2xx) responses are returned; others throw errors.

• Issue: Missing Newline at End of File
  The apiClient.ts file is missing a newline character at the end of the file.

  • Fix: Add a newline character at the end of frontend/services/apiClient.ts. This is a common linting rule.

What's Good ✅

1. Clear Intent and Problem Solving: The PR clearly identifies a critical problem (caching of error responses) and proposes a direct solution by validating HTTP status codes. This proactive approach to data integrity is commendable.
2. Helper Functions for HTTP Methods: The provision of get, post, put, and del helper functions is a good abstraction, making the API client easier and more consistent to use for common HTTP operations.
3. Robust Error Data Parsing: The use of response.json().catch(() => ({})) for error responses is a good practice to prevent further errors if the server returns a non-JSON error body.

Verdict

Request Changes

The PR introduces several High Severity issues, including syntax errors, runtime errors due to missing imports, and significant, unrelated architectural changes (audio-module removal, pnpm-workspace.yaml modifications) that could break the application or introduce unexpected behavior. The inconsistent error handling also needs to be addressed for maintainability. The most critical issue is the PROD_URL re-declaration and missing Constants import, which will prevent the application from running.

[SB2318]

@vaishnavi003-svg, please cover the bot points

[SB2318]

@vaishnavi003-svg please fix the bot points.

[vaishnavi003-svg]

@SB2318 Working on the bot review points, will push fixes shortly.

[vaishnavi003-svg]

@SB2318
All bot points have been addressed:

✅ PROD_URL - single declaration with Constants import
✅ ApiError class - consistent error handling
✅ pnpm-workspace.yaml - reverted to original
✅ Content-Type header - flexible for different request types
✅ 'any' replaced with 'unknown' for type safety
✅ Professional comments added
✅ Newline added at EOF
✅ PR description updated

Ready for final review!