Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Util function for cleaner usage of TanStack useQuery #9395

Merged
merged 11 commits into from
Dec 13, 2024

Conversation

amjithtitus09
Copy link
Contributor

@amjithtitus09 amjithtitus09 commented Dec 12, 2024

Proposed Changes

@ohcnetwork/care-fe-code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new components: PatientDetails, SearchMedicines, and FacilityDetails.
    • Enhanced API integration with TanStack Query, including a new centralized error handling mechanism.
    • Added a new interface for structured query options.
  • Bug Fixes

    • Simplified error handling logic for common scenarios, including session expiry and bad requests.
  • Documentation

    • Updated README to reflect changes in API integration and usage patterns.
    • Expanded migration guide for transitioning to the new API structure.
  • Chores

    • Improved the overall structure and clarity of documentation.

@amjithtitus09 amjithtitus09 requested a review from a team as a code owner December 12, 2024 12:33
Copy link
Contributor

coderabbitai bot commented Dec 12, 2024

Warning

Rate limit exceeded

@rithviknishad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 0 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 7283372 and f4aff63.

📒 Files selected for processing (1)
  • src/Utils/request/README.md (5 hunks)

Walkthrough

The pull request introduces significant updates to the documentation and implementation of CARE's data fetching utilities, transitioning to TanStack Query. Key changes include the introduction of new components, an enhanced error handling mechanism, and modifications to the QueryClient configuration. The README documentation has been updated to reflect new usage patterns, and a migration guide has been provided for transitioning from legacy methods. Additionally, new types and functions have been added to improve request handling and error management.

Changes

File Change Summary
src/Utils/request/README.md Updated documentation for TanStack Query, replaced FooDetails with UserProfile, added new components (PatientDetails, SearchMedicines, FacilityDetails), and expanded migration guide.
src/App.tsx Enhanced QueryClient configuration with queryCache, updated retry option from 3 to 2, and removed initialIsOpen prop from ReactQueryDevtools.
src/Utils/request/errorHandler.ts Introduced handleQueryError function for centralized error handling, checking for QueryError, session expiry, and bad requests.
src/Utils/request/request.ts Changed getResponseBody function to be exported for accessibility; core functionality remains unchanged.
src/Utils/request/types.ts Added new interface QueryOptions<TBody> for structured query handling.
src/Utils/request/query.ts Introduced queryRequest for API requests with TanStack Query and a query function for request execution with cancellation support.
src/Utils/request/queryError.ts Added QueryError class for detailed error handling, including properties for status and cause.

Possibly related PRs

Suggested labels

needs review, tested

Suggested reviewers

  • Jacobjeevan
  • shivankacker

Poem

🐇 In the land of code where rabbits play,
New queries hop in a bright array.
With error handling, smooth and neat,
Our data fetch dance is now complete!
So let’s celebrate, with joy and cheer,
For TanStack Query brings us near! 🎉


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Experiment)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit db9a303
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/675ad800ed0e8a000812edb4
😎 Deploy Preview https://deploy-preview-9395--care-ohc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit f4aff63
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/675c25471393f90008e876f2
😎 Deploy Preview https://deploy-preview-9395--care-ohc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
src/Utils/request/api-request.ts (1)

12-18: Enhance function documentation for better clarity.

While the function is correctly implemented, the documentation could be more comprehensive by:

  • Describing the purpose and return type
  • Documenting parameters and their usage
  • Adding example usage

Consider expanding the documentation:

 /**
  * Creates a TanStack Query compatible request function
+ *
+ * @param route - The API route configuration
+ * @param options - Optional configuration for the query
+ * @returns A function compatible with TanStack Query's queryFn
+ *
+ * @example
+ * ```tsx
+ * useQuery({
+ *   queryKey: ['users'],
+ *   queryFn: api.query(routes.users.list)
+ * });
+ * ```
  */
src/Utils/request/README.md (3)

13-60: Improve error handling in examples.

While the examples are comprehensive, the error handling in FacilityDetails could be improved:

  1. It doesn't handle the case where response is undefined
  2. Direct navigation on error might not be the best user experience

Consider this improved version:

 function FacilityDetails({ id }: { id: string }) {
-  const { data: response } = useQuery({
+  const { data: response, error } = useQuery({
     queryKey: ['facility', id],
     queryFn: api.query(routes.getFacility, {
       pathParams: { id }
-    })
+    }),
+    useErrorBoundary: true
   });

-  if (response?.res && !response.res.ok) {
+  if (error) {
+    return <ErrorComponent error={error} onRetry={() => refetch()} />;
+  }
+
+  if (!response?.res?.ok) {
     navigate('/not-found');
+    return null;
   }

   return <div>{response?.data?.name}</div>;
 }

Line range hint 62-159: Add section on error handling best practices.

The migration guide is comprehensive but could benefit from a dedicated section on error handling best practices with TanStack Query.

Consider adding:

### Error Handling Best Practices

1. **Global Error Handling**:
```tsx
// Configure error handling globally
const queryClient = new QueryClient({
  defaultOptions: {
    queries: {
      useErrorBoundary: (error) => error.status >= 500,
      retry: (failureCount, error) => {
        if (error.status === 404) return false;
        return failureCount < 3;
      }
    }
  }
});
  1. Component-Level Error Handling:
function Component() {
  const { error, isError } = useQuery({
    queryKey: ['key'],
    queryFn: api.query(route),
    onError: (error) => {
      if (error.status === 401) {
        refreshToken();
      }
    }
  });

  if (isError) {
    return <ErrorBoundary error={error} />;
  }
}

---

Line range hint `161-276`: **Add deprecation notice for legacy support.**

Consider adding a clear deprecation notice for the legacy pattern to encourage migration to the new approach.

Add at the start of the Legacy Support section:

```markdown
## Legacy Support: `useTanStackQueryInstead`

> ⚠️ **Deprecation Notice**: This pattern is maintained for backward compatibility but is considered legacy. New code should use the modern TanStack Query pattern with `api.query` as described in the sections above. Consider migrating existing code when making substantial changes.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a3a59f and b41e19e.

📒 Files selected for processing (2)
  • src/Utils/request/README.md (1 hunks)
  • src/Utils/request/api-request.ts (1 hunks)
🔇 Additional comments (3)
src/Utils/request/api-request.ts (3)

1-10: LGTM! Well-structured interface with type safety.

The QueryOptions interface is well-designed with:

  • Generic type parameter for response data
  • Optional parameters for flexibility
  • Clear separation of path and query parameters

19-32: LGTM! Clean implementation with proper signal handling.

The implementation:

  • Correctly accepts and forwards AbortSignal
  • Properly propagates all query options
  • Maintains type safety throughout

34-38: LGTM! Clean export with proper type inference.

Good use of as const for better type safety.

src/Utils/request/api-request.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (5)
src/Utils/request/errorHandler.ts (2)

7-9: Consider using TypeScript discriminated unions for error types

The type casting to any reduces type safety. Consider defining proper error types using TypeScript discriminated unions.

type QueryError = 
  | { name: 'AbortError' }
  | { code: 'token_not_valid' | 'not_found'; detail?: string }
  | { status: number; detail?: string; silent?: boolean };

export function handleQueryError(error: Error | QueryError) {
  const err = error as QueryError;  // More type-safe cast
  // ... rest of the implementation
}

31-34: Consider using a custom error message map

The hardcoded error message "Something went wrong!" could be moved to a centralized message map for better maintainability and i18n support.

const ERROR_MESSAGES = {
  DEFAULT: 'Something went wrong!',
  // Add more specific messages here
} as const;

// Then use:
notify.Error({ msg: err?.detail || ERROR_MESSAGES.DEFAULT });
src/Utils/request/api-request.ts (1)

28-39: Consider implementing retry logic for network failures

The error handling focuses on HTTP status codes but doesn't specifically handle network failures. Consider implementing retry logic for transient network issues.

const MAX_RETRIES = 3;
const RETRY_DELAY = 1000;

async function withRetry<T>(fn: () => Promise<T>, retries = MAX_RETRIES): Promise<T> {
  try {
    return await fn();
  } catch (error) {
    if (retries > 0 && error instanceof TypeError && error.message === 'Failed to fetch') {
      await new Promise(resolve => setTimeout(resolve, RETRY_DELAY));
      return withRetry(fn, retries - 1);
    }
    throw error;
  }
}

// Usage in queryRequest:
const res = await withRetry(() => fetch(url, requestOptions));
src/Utils/request/request.ts (1)

Line range hint 64-82: Consider enhancing error handling in getResponseBody

While the function handles different content types well, there are a few improvements that could be made:

  1. The content-length check could be more explicit
  2. The error handling could be more informative

Consider this enhancement:

 export async function getResponseBody<TData>(res: Response): Promise<TData> {
-  if (!(res.headers.get("content-length") !== "0")) {
+  const contentLength = res.headers.get("content-length");
+  if (!contentLength || contentLength === "0") {
     return null as TData;
   }

   const isJson = res.headers.get("content-type")?.includes("application/json");
   const isImage = res.headers.get("content-type")?.includes("image");

   if (isImage) {
     return (await res.blob()) as TData;
   }

   if (!isJson) {
     return (await res.text()) as TData;
   }

   try {
     return await res.json();
   } catch (error) {
-    return (await res.text()) as TData;
+    console.warn(`Failed to parse JSON response: ${error}. Falling back to text.`);
+    const textResponse = await res.text();
+    return textResponse as TData;
   }
 }
src/Utils/request/README.md (1)

92-101: Consider adding error handling examples with custom logic

While the global error handling documentation is good, it would be helpful to show examples of custom error handling for specific use cases.

Consider adding an example like:

function CustomErrorHandling() {
  const { data, error } = useQuery({
    queryKey: ['custom-error'],
    queryFn: api.query(routes.custom, {
      silent: true // Suppress global handling
    }),
    onError: (error) => {
      if (error.status === 409) {
        // Handle conflict error
        showConflictResolutionDialog();
      }
    }
  });
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b41e19e and 7ae322b.

📒 Files selected for processing (5)
  • src/App.tsx (3 hunks)
  • src/Utils/request/README.md (5 hunks)
  • src/Utils/request/api-request.ts (1 hunks)
  • src/Utils/request/errorHandler.ts (1 hunks)
  • src/Utils/request/request.ts (1 hunks)
🔇 Additional comments (4)
src/Utils/request/api-request.ts (1)

8-12: LGTM: Well-defined TypeScript interface

The QueryOptions interface is well-structured and provides good type safety for the request parameters.

src/App.tsx (1)

31-33: LGTM: Proper integration of error handling

The integration of handleQueryError through QueryCache is the recommended approach for centralized error handling in TanStack Query.

src/Utils/request/README.md (2)

7-89: LGTM! Well-structured documentation with clear examples

The documentation effectively demonstrates the usage of api.query with various scenarios including basic usage, path parameters, query parameters, and error handling.


165-165: Verify consistency in queryFn examples

The examples in the Common Patterns section should be consistent with the main documentation.

Let's verify the usage patterns:

Also applies to: 181-184, 197-200

✅ Verification successful

All queryFn examples in the documentation are consistent

Based on the search results, all queryFn examples in the documentation follow the same pattern:

  • Basic usage: queryFn: api.query(route)
  • With params: queryFn: api.query(route, { pathParams, queryParams })
  • With options: queryFn: api.query(route, { silent: true })

The examples in the Common Patterns section align perfectly with the main documentation, showing consistent usage patterns throughout.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all queryFn usages to ensure consistency
rg -A 2 'queryFn:' src/

Length of output: 1918

Comment on lines 45 to 49
function handleSessionExpiry() {
if (!location.pathname.startsWith("/session-expired")) {
navigate(`/session-expired?redirect=${window.location.href}`);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

URL encoding required for session expiry redirect parameter

The codebase shows consistent usage of URL encoding in other parts where query parameters contain URLs or text (e.g., in DoctorVideoSlideover.tsx). The current implementation in errorHandler.ts directly uses window.location.href without encoding, which could cause issues with special characters in the URL.

  • src/Utils/request/errorHandler.ts: Add URL encoding using encodeURIComponent(window.location.href)
  • src/Utils/request/handleResponse.ts: Same change needed for consistency
🔗 Analysis chain

Verify URL encoding in session expiry redirect

The current implementation directly uses window.location.href in the redirect URL, which might need URL encoding.


Let me check if there are any existing implementations of URL encoding in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other redirect implementations to verify consistency
rg -A 2 "redirect.*window\.location"

Length of output: 417


Script:

#!/bin/bash
# Search for encodeURI or encodeURIComponent usage
rg -A 2 "encodeURI(Component)?"

# Search for any URL encoding related utility functions
ast-grep --pattern 'function $name($$$) { 
  $$$ 
  encodeURI($$$)
  $$$
}'

Length of output: 764

Comment on lines 47 to 54
function createQuery<TData, TBody>(
route: Route<TData, TBody>,
options?: QueryOptions,
) {
return async ({ signal }: { signal?: AbortSignal } = {}) => {
return queryRequest(route, { ...options, signal });
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding request body support

The createQuery function doesn't currently support request bodies for POST/PUT operations. Consider adding this capability.

function createQuery<TData, TBody>(
  route: Route<TData, TBody>,
  options?: QueryOptions & { body?: TBody }
) {
  return async ({ signal }: { signal?: AbortSignal } = {}) => {
    const requestOptions = {
      ...options,
      signal,
      ...(options?.body && {
        body: JSON.stringify(options.body),
        headers: {
          'Content-Type': 'application/json',
          ...makeHeaders(route.noAuth ?? false),
        },
      }),
    };
    return queryRequest(route, requestOptions);
  };
}

src/Utils/request/api-request.ts Outdated Show resolved Hide resolved
src/Utils/request/api-request.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (2)
src/Utils/response/responseError.ts (1)

22-27: Simplify the silent property type

The type boolean | false is redundant since false is already included in boolean.

Apply this diff:

 export interface ErrorCause {
   code: string;
   status: number;
-  silent: boolean | false;
+  silent?: boolean;
   detail: string;
 }
src/Utils/request/api-request.ts (1)

31-41: Handle undefined error name property

The error name property might be undefined when constructing ResponseError.

Apply this diff:

     throw new ResponseError({
-      name: error.name,
+      name: error.name || 'ApiError',
       message: "Request Failed",
       cause: {
         ...error,
         code: error.code || (res.status === 404 ? "not_found" : undefined),
         status: res.status,
         silent: options?.silent,
         detail: error.detail || "Something went wrong!",
       },
     });
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7ae322b and 0c6f096.

📒 Files selected for processing (4)
  • src/Utils/request/api-request.ts (1 hunks)
  • src/Utils/request/errorHandler.ts (1 hunks)
  • src/Utils/request/types.ts (1 hunks)
  • src/Utils/response/responseError.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Utils/request/errorHandler.ts
🔇 Additional comments (1)
src/Utils/request/api-request.ts (1)

50-57: 🛠️ Refactor suggestion

Enhance createQuery to fully support request body

As mentioned in previous reviews, createQuery should better support request bodies for POST/PUT operations.

Apply this diff:

 function createQuery<TData, TBody>(
   route: Route<TData, TBody>,
-  options?: QueryOptions,
+  options?: QueryOptions<TBody>,
 ) {
   return async ({ signal }: { signal?: AbortSignal } = {}) => {
     return queryRequest(route, { ...options, signal });
   };
 }

src/Utils/response/responseError.ts Outdated Show resolved Hide resolved
src/Utils/request/types.ts Outdated Show resolved Hide resolved
src/Utils/request/api-request.ts Outdated Show resolved Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
src/Utils/request/api-request.ts (2)

54-61: Improve type safety and API design

Consider these improvements:

  1. Move signal into QueryOptions interface for consistency
  2. Add explicit return type annotation for better type safety

Consider applying these changes:

+type QueryFn<TData> = (options?: { signal?: AbortSignal }) => Promise<TData>;

 function createQuery<TData, TBody>(
   route: Route<TData, TBody>,
-  options?: QueryOptions,
+  options?: QueryOptions & { signal?: AbortSignal },
- ) {
-  return async ({ signal }: { signal?: AbortSignal } = {}) => {
-    return queryRequest(route, { ...options, signal });
-  };
+ ): QueryFn<TData> {
+  return async (queryOptions = {}) => {
+    return queryRequest(route, { ...options, signal: queryOptions?.signal });
+  };
 }

24-24: Fix string literal quotes style

Use double quotes for consistency with project style guidelines.

-      'Content-Type': 'application/json'
+      "Content-Type": "application/json"
🧰 Tools
🪛 eslint

[error] 24-24: Replace 'Content-Type':·'application/json' with "Content-Type":·"application/json",

(prettier/prettier)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c6f096 and 015a007.

📒 Files selected for processing (1)
  • src/Utils/request/api-request.ts (1 hunks)
🧰 Additional context used
🪛 eslint
src/Utils/request/api-request.ts

[error] 24-24: Replace 'Content-Type':·'application/json' with "Content-Type":·"application/json",

(prettier/prettier)

🔇 Additional comments (1)
src/Utils/request/api-request.ts (1)

63-67: LGTM!

The api object export is well-typed and follows TypeScript best practices with the as const assertion.

src/Utils/request/api-request.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
src/Utils/request/api-request.ts (1)

51-61: Consider improving function signature and documentation

  1. The signal parameter could be part of QueryOptions instead of being passed separately
  2. The documentation could be more detailed about usage patterns

Consider this enhancement:

-function createQuery<TData, TBody>(
-  route: Route<TData, TBody>,
-  options?: QueryOptions<TBody>,
-) {
-  return ({ signal }: { signal?: AbortSignal } = {}) => {
-    return queryRequest(route, { ...options, signal });
-  };
-}
+/**
+ * Creates a TanStack Query compatible request function
+ * @param route - The API route configuration
+ * @param options - Query options including body, params, etc.
+ * @returns A function compatible with TanStack Query's queryFn
+ * @example
+ * const useUserQuery = (id: string) => useQuery({
+ *   queryKey: ['user', id],
+ *   queryFn: api.query({ 
+ *     path: '/api/users/:id',
+ *     method: 'GET'
+ *   }, {
+ *     pathParams: { id }
+ *   })
+ * })
+ */
+function createQuery<TData, TBody>(
+  route: Route<TData, TBody>,
+  options?: QueryOptions<TBody> & { signal?: AbortSignal },
+) {
+  return () => queryRequest(route, options);
+}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 015a007 and e6603ff.

📒 Files selected for processing (2)
  • src/Utils/request/api-request.ts (1 hunks)
  • src/Utils/request/types.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Utils/request/types.ts
🔇 Additional comments (3)
src/Utils/request/api-request.ts (3)

1-8: LGTM! Clean import structure

The imports are well-organized and each import serves a clear purpose in the implementation.


63-67: LGTM! Clean export implementation

The export uses TypeScript's const assertion correctly to ensure type safety.


29-46: 🛠️ Refactor suggestion

Enhance error handling robustness

The current error handling implementation could be more robust. Consider implementing:

  1. Network failure handling
  2. Request timeout
  3. Retry mechanism for transient failures
  4. Remove duplicate error message

Apply this enhancement:

-  const res = await fetch(url, requestOptions);
+  try {
+    const controller = new AbortController();
+    const timeoutId = setTimeout(() => controller.abort(), 30000); // 30s timeout
+    
+    const res = await fetch(url, {
+      ...requestOptions,
+      signal: requestOptions.signal || controller.signal,
+    });
+    
+    clearTimeout(timeoutId);
+
+    if (!res.ok) {
+      const error = await res
+        .json()
+        .catch(() => ({ detail: "Failed to parse error response" }));
+      
+      throw new ResponseError({
+        name: error.name,
+        message: "Request Failed",
+        cause: {
+          ...error,
+          code: error.code || (res.status === 404 ? "not_found" : undefined),
+          status: res.status,
+          silent: options?.silent,
+          detail: error.detail || "Failed to complete request"
+        }
+      });
+    }
+    
+    return getResponseBody<TData>(res);
+  } catch (error) {
+    if (error instanceof ResponseError) throw error;
+    
+    throw new ResponseError({
+      name: "NetworkError",
+      message: "Request Failed",
+      cause: {
+        detail: error instanceof Error ? error.message : "Network request failed",
+        status: 0,
+        silent: options?.silent
+      }
+    });
+  }

Likely invalid or redundant comment.

…revent session expiry if silenced, and other cleanups
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
src/Utils/request/queryError.ts (1)

1-1: Consider making QueryErrorCause more specific

The current type allows any unknown values. Consider defining a more specific structure based on your API's error response format to improve type safety and developer experience.

-type QueryErrorCause = Record<string, unknown> | undefined;
+type QueryErrorCause = {
+  code?: string;
+  detail?: string;
+  errors?: Record<string, string[]>;
+} | undefined;
src/Utils/request/query.ts (1)

8-37: Consider enhancing error messages with more context

The error handling is well-implemented, but the generic "Request Failed" message could be more specific.

     throw new QueryError({
-      message: "Request Failed",
+      message: `${method} request to ${path} failed with status ${res.status}`,
       status: res.status,
       silent: options?.silent ?? false,
       cause: data as unknown as Record<string, unknown>,
     });
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e6603ff and 5a7510c.

📒 Files selected for processing (4)
  • src/App.tsx (3 hunks)
  • src/Utils/request/errorHandler.ts (1 hunks)
  • src/Utils/request/query.ts (1 hunks)
  • src/Utils/request/queryError.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/App.tsx
🔇 Additional comments (4)
src/Utils/request/queryError.ts (1)

3-24: LGTM! Well-structured error class implementation

The QueryError class follows best practices:

  • Properly extends Error with cause support
  • Clear typing for all properties
  • Well-structured constructor with typed parameters
src/Utils/request/errorHandler.ts (2)

6-35: LGTM! Well-structured error handling flow

The error handling implementation is comprehensive and well-organized:

  • Proper AbortError handling
  • Type checking for QueryError
  • Silent error handling
  • Specific error cases (session expiry, bad request)

46-50: ⚠️ Potential issue

URL encoding required for session expiry redirect parameter

The redirect URL should be encoded to handle special characters safely.

-    navigate(`/session-expired?redirect=${window.location.href}`);
+    navigate(`/session-expired?redirect=${encodeURIComponent(window.location.href)}`);
src/Utils/request/query.ts (1)

42-49: LGTM! Well-designed TanStack Query integration

The query function is well-implemented:

  • Clean interface for TanStack Query
  • Proper type propagation
  • Correct signal handling

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
src/Utils/request/query.ts (4)

28-30: Enhance network error handling with more specific information

The generic "Network Error" message doesn't provide enough context for debugging. Consider including the original error details.

-  } catch {
+  } catch (error) {
-    throw new Error("Network Error");
+    throw new Error(`Network Error: ${error instanceof Error ? error.message : 'Unknown error'}`);

34-41: Consider logging mechanism for silent errors

While the silent option is useful for suppressing UI errors, consider adding a logging mechanism for these errors to aid debugging.

Consider implementing a logging service that captures these errors even when they're silently handled.


46-56: LGTM! Consider enhancing documentation

The implementation effectively wraps the queryRequest function for TanStack Query compatibility. The signal handling is clean and type-safe.

Consider expanding the documentation to include:

  • Usage example
  • Description of type parameters
  • Explanation of the signal handling
 /**
  * Creates a TanStack Query compatible request function
+ * 
+ * @example
+ * const useUserQuery = (userId: string) => useQuery({
+ *   queryKey: ['user', userId],
+ *   queryFn: query({ path: `/users/${userId}`, method: 'GET' })
+ * })
+ * 
+ * @template TData The expected response data type
+ * @template TBody The request body type
+ * @param route The route configuration
+ * @param options Additional query options
+ * @returns A function compatible with TanStack Query's queryFn
  */

1-56: Consider implementing request optimization strategies

As this is a core request utility, consider implementing:

  1. Rate limiting mechanism to prevent API abuse
  2. Request deduplication for identical concurrent requests
  3. Response caching strategy aligned with TanStack Query's caching

These optimizations would improve application performance and reduce server load.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a7510c and 7283372.

📒 Files selected for processing (2)
  • src/App.tsx (2 hunks)
  • src/Utils/request/query.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/App.tsx
🔇 Additional comments (2)
src/Utils/request/query.ts (2)

1-7: LGTM! Well-organized imports

The imports are logically organized and all are necessary for the implementation.


39-39: Review type casting safety

The type casting data as unknown as Record<string, unknown> might be unsafe. Consider adding runtime type validation or a more specific type guard.

✅ Verification successful

Type casting is appropriate in this context

The type casting is actually safe and appropriate here because:

  1. The QueryError class explicitly defines cause as QueryErrorCause = Record<string, unknown> | undefined
  2. The casting is used in an error handling context where the API response data is being passed as the error cause
  3. The getResponseBody function handles different response types (JSON, images, etc.) and returns them as the generic type TData, making the intermediate unknown cast necessary
  4. This pattern is consistently used across the request handling utilities
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for similar type castings in the codebase that might need attention
rg "as unknown as Record<string, unknown>" -A 2 -B 2

Length of output: 324


Script:

#!/bin/bash
# Let's check the context of this type casting by examining the entire function
ast-grep --pattern 'function $$$($$$) {
  $$$
  cause: data as unknown as Record<string, unknown>,
  $$$
}'

# Also check where this data comes from and how it's used
rg "data.*as.*Record" -B 10 -A 5

Length of output: 1728


Script:

#!/bin/bash
# Let's check the implementation of getResponseBody to understand the data type
ast-grep --pattern 'function getResponseBody$$$($$$) {
  $$$
}'

# Also check the QueryError class definition
ast-grep --pattern 'class QueryError $$$'

Length of output: 109


Script:

#!/bin/bash
# Let's try a different pattern to find getResponseBody implementation
rg "getResponseBody" -B 3 -A 10

# And find QueryError definition
rg "class QueryError" -B 3 -A 10

# Also check if there's any type definition for the error response
rg "type.*Error" -A 5

Length of output: 6725

@rithviknishad rithviknishad merged commit 05e8f4c into ohcnetwork:develop Dec 13, 2024
19 checks passed
Copy link

@amjithtitus09 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants